RESOLVED FIXED 189213
Add infrastructure to dump resource load statistics
https://bugs.webkit.org/show_bug.cgi?id=189213
Summary Add infrastructure to dump resource load statistics
Woodrow Wang
Reported 2018-08-31 15:31:51 PDT
Added dumping function for testing resource load statistics
Attachments
Patch (16.32 KB, patch)
2018-08-31 15:43 PDT, Woodrow Wang
no flags
Patch (19.49 KB, patch)
2018-08-31 17:07 PDT, Woodrow Wang
no flags
Patch (19.49 KB, patch)
2018-08-31 17:40 PDT, Woodrow Wang
no flags
Patch (18.79 KB, patch)
2018-09-04 15:41 PDT, Woodrow Wang
no flags
Patch (18.81 KB, patch)
2018-09-04 16:18 PDT, Woodrow Wang
no flags
Patch (18.83 KB, patch)
2018-09-04 16:33 PDT, Woodrow Wang
no flags
Patch (18.85 KB, patch)
2018-09-04 17:27 PDT, Woodrow Wang
no flags
Archive of layout-test-results from ews203 for win-future (12.83 MB, application/zip)
2018-09-04 22:27 PDT, EWS Watchlist
no flags
Patch (18.83 KB, patch)
2018-09-05 09:30 PDT, Woodrow Wang
no flags
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.49 MB, application/zip)
2018-09-05 10:48 PDT, EWS Watchlist
no flags
Patch (18.83 KB, patch)
2018-09-05 11:28 PDT, Woodrow Wang
no flags
Woodrow Wang
Comment 1 2018-08-31 15:43:57 PDT
Woodrow Wang
Comment 2 2018-08-31 17:07:03 PDT
Woodrow Wang
Comment 3 2018-08-31 17:40:21 PDT
Daniel Bates
Comment 4 2018-09-04 14:07:40 PDT
Comment on attachment 348690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348690&action=review > Source/WebCore/ChangeLog:3 > + Added dumping function for testing resource load statistics Maybe a better title would be: Add infrastructure to dump resource load statistics > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155 > + store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) { This is ref'ing the passed string and this function does not need to take ownership of the string. I suggest the callback take a "const String&" as the parameter type. Can we come up with a better name for the parameter? Maybe resourceLoadStatistics? > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > + callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context); Is there a better way to convert from WTF::String to WKStringRef. I am surprised that this problem has not already been solved. Does it make sense to to use toCopiedAPI()? > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678 > + Please remove the leading whitespace from this line. > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679 > + StringBuilder dumpResourceLoadStatisticsMapString; Can we come up with a better name for this variable? Maybe result? > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500 > + Please remove the leading whitespace from this line. > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502 > + String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString(); Can we come up with a better name for name of the local variable? Maybe result? or resourceLoadStatistics? > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271 > + void setDumpResourceLoadStatistics(boolean shouldDump); I suggest that we take a similar approach as the other dumping function is this file and call this function dumpResourceLoadStatistics() and it should not take any arguments. We should also group this declaration with the declarations of the other dumping functions at the top of this file. > Tools/WebKitTestRunner/TestController.cpp:2808 > + WKStringRef dump; Can we come up with a more descriptive name for this?
Woodrow Wang
Comment 5 2018-09-04 15:23:15 PDT
(In reply to Daniel Bates from comment #4) > Comment on attachment 348690 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348690&action=review > > > Source/WebCore/ChangeLog:3 > > + Added dumping function for testing resource load statistics > > Maybe a better title would be: Add infrastructure to dump resource load > statistics Agreed, changing accordingly. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:155 > > + store->dumpResourceLoadStatistics([context, callback] (String dumpResourceLoadStatistics) { > > This is ref'ing the passed string and this function does not need to take > ownership of the string. I suggest the callback take a "const String&" as > the parameter type. Can we come up with a better name for the parameter? > Maybe resourceLoadStatistics? Fixed. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > > + callback(WKStringCreateWithUTF8CString(dumpResourceLoadStatistics.utf8().data()), context); > > Is there a better way to convert from WTF::String to WKStringRef. I am > surprised that this problem has not already been solved. Does it make sense > to to use toCopiedAPI()? Yes, toCopiedAPI() is a utility function we can use. > > > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:678 > > + > > Please remove the leading whitespace from this line. Done. > > > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:679 > > + StringBuilder dumpResourceLoadStatisticsMapString; > > Can we come up with a better name for this variable? Maybe result? Changed. > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:500 > > + > > Please remove the leading whitespace from this line. Done. > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:502 > > + String dumpResourceLoadStatistics = m_memoryStore ? m_memoryStore->dumpResourceLoadStatistics() : emptyString(); > > Can we come up with a better name for name of the local variable? Maybe > result? or resourceLoadStatistics? Changed to resourceLoadStatistics. > > > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:271 > > + void setDumpResourceLoadStatistics(boolean shouldDump); > > I suggest that we take a similar approach as the other dumping function is > this file and call this function dumpResourceLoadStatistics() and it should > not take any arguments. We should also group this declaration with the > declarations of the other dumping functions at the top of this file. > Done. > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef dump; > > Can we come up with a more descriptive name for this? Changed to resourceLoadStatisticsRepresentation.
Woodrow Wang
Comment 6 2018-09-04 15:41:25 PDT
Chris Dumez
Comment 7 2018-09-04 15:50:53 PDT
Comment on attachment 348859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348859&action=review > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151 > + callback(WKStringCreateWithUTF8CString(""), context); It seems odd to me that we're transferring ownership of the WKString to the callback here. I think we usually do not transfer ownership of such string to callback functions. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > + callback(WebKit::toCopiedAPI(resourceLoadStatistics), context); Ditto. I would have used: toAPI(resourceLoadStatistics.impl()) > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497 > +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler) Why String and not const String& ? > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503 > + postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable { you are failing to isolate copy result before passing it to the other thread here. You want "result = result.isolatedCopy()". > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); It looks to me that the value returned by dumpResourceLoadStatistics() was leaked here.
Daniel Bates
Comment 8 2018-09-04 16:09:52 PDT
Comment on attachment 348859 [details] Patch r- per comment #7.
Woodrow Wang
Comment 9 2018-09-04 16:11:33 PDT
(In reply to Chris Dumez from comment #7) > Comment on attachment 348859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348859&action=review Thanks for the tips/help! > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:151 > > + callback(WKStringCreateWithUTF8CString(""), context); > > It seems odd to me that we're transferring ownership of the WKString to the > callback here. I think we usually do not transfer ownership of such string > to callback functions. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:156 > > + callback(WebKit::toCopiedAPI(resourceLoadStatistics), context); > > Ditto. I would have used: > toAPI(resourceLoadStatistics.impl()) Fixed to use toAPI instead of toCopiedAPI > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:497 > > +void WebResourceLoadStatisticsStore::dumpResourceLoadStatistics(CompletionHandler<void(String)>&& completionHandler) > > Why String and not const String& ? Changed to use const String& > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:503 > > + postTaskReply([result, completionHandler = WTFMove(completionHandler)] () mutable { > > you are failing to isolate copy result before passing it to the other thread > here. You want "result = result.isolatedCopy()". Thanks for pointing this out! > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > It looks to me that the value returned by dumpResourceLoadStatistics() was > leaked here.
Woodrow Wang
Comment 10 2018-09-04 16:18:55 PDT
Woodrow Wang
Comment 11 2018-09-04 16:33:21 PDT
Chris Dumez
Comment 12 2018-09-04 16:33:40 PDT
Comment on attachment 348864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348864&action=review > Tools/WebKitTestRunner/TestController.cpp:2808 > + WKStringRef resourceLoadStatisticsRepresentation; This should probably be a WKRetainPtr<WKStringRef> so that your string stays alive. > Tools/WebKitTestRunner/TestController.cpp:2879 > +WKStringRef TestController::dumpResourceLoadStatistics() Maybe this could this return a String instead of a WKStringRef ? > Tools/WebKitTestRunner/TestController.cpp:2885 > + return context.resourceLoadStatisticsRepresentation; return context.resourceLoadStatisticsRepresentation.get(); if you make the change I suggest above. > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); Is this really the right place to dump the statistics? Seems unrelated to resetting state?
Woodrow Wang
Comment 13 2018-09-04 16:46:34 PDT
(In reply to Chris Dumez from comment #12) > Comment on attachment 348864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef resourceLoadStatisticsRepresentation; > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > alive. Fixed! > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > Maybe this could this return a String instead of a WKStringRef ? Agreed that it seems cleaner this way. Modified here and call sites accordingly. > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > + return context.resourceLoadStatisticsRepresentation; > > return context.resourceLoadStatisticsRepresentation.get(); > > if you make the change I suggest above. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > Is this really the right place to dump the statistics? Seems unrelated to > resetting state? The design of this dumping function only allows and intends for the resource load statistics map to be dumped once at the conclusion of a test. At that point in the test, however, if the test calls resetStatisticsToConsistentState, the resource load statistics map will be cleared, and so the dumped string will be empty. Thus, right now we can simply record the state of the resource load statistics map at the time of resetting the statistics and store it in the instance variable m_savedResourceLoadStatistics.
Woodrow Wang
Comment 14 2018-09-04 17:25:08 PDT
(In reply to Chris Dumez from comment #12) > Comment on attachment 348864 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > + WKStringRef resourceLoadStatisticsRepresentation; > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > alive. > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > Maybe this could this return a String instead of a WKStringRef ? Is there a reason we should return a String over a WKStringRef? It seems like in the existing code that the precedence is to use WKStringRefs in the TestController. > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > + return context.resourceLoadStatisticsRepresentation; > > return context.resourceLoadStatisticsRepresentation.get(); > > if you make the change I suggest above. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > Is this really the right place to dump the statistics? Seems unrelated to > resetting state?
Woodrow Wang
Comment 15 2018-09-04 17:27:38 PDT
Chris Dumez
Comment 16 2018-09-04 18:23:48 PDT
(In reply to Woodrow Wang from comment #14) > (In reply to Chris Dumez from comment #12) > > Comment on attachment 348864 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=348864&action=review > > > > > Tools/WebKitTestRunner/TestController.cpp:2808 > > > + WKStringRef resourceLoadStatisticsRepresentation; > > > > This should probably be a WKRetainPtr<WKStringRef> so that your string stays > > alive. > > > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > > +WKStringRef TestController::dumpResourceLoadStatistics() > > > > Maybe this could this return a String instead of a WKStringRef ? > > Is there a reason we should return a String over a WKStringRef? It seems > like in the existing code that the precedence is to use WKStringRefs in the > TestController. I do not feel strongly but it seems your call site seems to want a String anyway. Also a String is much nicer to deal with than a WKStringRef because you do not have to worry about lifetime. > > > > > Tools/WebKitTestRunner/TestController.cpp:2885 > > > + return context.resourceLoadStatisticsRepresentation; > > > > return context.resourceLoadStatisticsRepresentation.get(); > > > > if you make the change I suggest above. > > > > > Tools/WebKitTestRunner/TestInvocation.cpp:1363 > > > + m_savedResourceLoadStatistics = toWTFString(TestController::singleton().dumpResourceLoadStatistics()); > > > > Is this really the right place to dump the statistics? Seems unrelated to > > resetting state?
EWS Watchlist
Comment 17 2018-09-04 22:27:29 PDT
Comment on attachment 348877 [details] Patch Attachment 348877 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9096872 New failing tests: fast/workers/worker-exception-during-navigation.html
EWS Watchlist
Comment 18 2018-09-04 22:27:42 PDT
Created attachment 348895 [details] Archive of layout-test-results from ews203 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews203 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Chris Dumez
Comment 19 2018-09-05 08:47:10 PDT
Comment on attachment 348877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348877&action=review LGTM with a couple of comments. > Tools/WebKitTestRunner/TestController.cpp:2879 > +WKStringRef TestController::dumpResourceLoadStatistics() nit: I think it would look nicer and be less error-prone in terms of lifetime management if this returned a String. Both your call sites need a String anyway. > Tools/WebKitTestRunner/TestInvocation.cpp:254 > + m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics); AFAICT, the empty string is a valid value that can be returned by dumpResourceLoadStatistics(). Therefore, I think the check for the ternary should be m_savedResourceLoadStatistics.isNull(). > Tools/WebKitTestRunner/TestInvocation.cpp:1361 > if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) { FYI, longer term, I think we should probably drop this TestRunner method. We should not rely on the test to go back to a consistent state, TestController should reset to a consistent state between tests by itself.
Woodrow Wang
Comment 20 2018-09-05 09:28:45 PDT
(In reply to Chris Dumez from comment #19) > Comment on attachment 348877 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348877&action=review > > LGTM with a couple of comments. > > > Tools/WebKitTestRunner/TestController.cpp:2879 > > +WKStringRef TestController::dumpResourceLoadStatistics() > > nit: I think it would look nicer and be less error-prone in terms of > lifetime management if this returned a String. Both your call sites need a > String anyway. Thanks for the help with avoiding memory leaks with the WKStringRef! > > > Tools/WebKitTestRunner/TestInvocation.cpp:254 > > + m_textOutput.append(m_savedResourceLoadStatistics.isEmpty() ? toWTFString(TestController::singleton().dumpResourceLoadStatistics()) : m_savedResourceLoadStatistics); > > AFAICT, the empty string is a valid value that can be returned by > dumpResourceLoadStatistics(). Therefore, I think the check for the ternary > should be m_savedResourceLoadStatistics.isNull(). This makes sense, so I'll change it. > > > Tools/WebKitTestRunner/TestInvocation.cpp:1361 > > if (WKStringIsEqualToUTF8CString(messageName, "StatisticsResetToConsistentState")) { > > FYI, longer term, I think we should probably drop this TestRunner method. We > should not rely on the test to go back to a consistent state, TestController > should reset to a consistent state between tests by itself. Agreed, this call to dumpResourceLoadStatistics() is replicated in the dumpResults() function, as the latter call site is much more logical. As an artifact of the resource load statistics' state being cleared by this function, we have this slightly more complex logic to save that state for dumping at the end of a test. Longer term, we can let the TestController handle the resetting of state by itself, making this code slightly simpler.
Woodrow Wang
Comment 21 2018-09-05 09:30:57 PDT
Chris Dumez
Comment 22 2018-09-05 09:42:11 PDT
Comment on attachment 348930 [details] Patch LGTM
Chris Dumez
Comment 23 2018-09-05 09:56:25 PDT
(In reply to Chris Dumez from comment #22) > Comment on attachment 348930 [details] > Patch > > LGTM r=me
EWS Watchlist
Comment 24 2018-09-05 10:48:14 PDT
Comment on attachment 348930 [details] Patch Attachment 348930 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9103565 New failing tests: accessibility/smart-invert-reference.html
EWS Watchlist
Comment 25 2018-09-05 10:48:16 PDT
Created attachment 348942 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Woodrow Wang
Comment 26 2018-09-05 11:28:30 PDT
WebKit Commit Bot
Comment 27 2018-09-05 14:25:14 PDT
Comment on attachment 348949 [details] Patch Clearing flags on attachment: 348949 Committed r235690: <https://trac.webkit.org/changeset/235690>
WebKit Commit Bot
Comment 28 2018-09-05 14:25:15 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 29 2018-09-05 14:27:29 PDT
Note You need to log in before you can comment on or make changes to this bug.