WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.49 KB, patch)
2018-08-31 17:07 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Patch
(19.49 KB, patch)
2018-08-31 17:40 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Patch
(18.79 KB, patch)
2018-09-04 15:41 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Patch
(18.81 KB, patch)
2018-09-04 16:18 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Patch
(18.83 KB, patch)
2018-09-04 16:33 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Patch
(18.85 KB, patch)
2018-09-04 17:27 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(18.83 KB, patch)
2018-09-05 09:30 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(18.83 KB, patch)
2018-09-05 11:28 PDT
,
Woodrow Wang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Woodrow Wang
Comment 1
2018-08-31 15:43:57 PDT
Created
attachment 348679
[details]
Patch
Woodrow Wang
Comment 2
2018-08-31 17:07:03 PDT
Created
attachment 348683
[details]
Patch
Woodrow Wang
Comment 3
2018-08-31 17:40:21 PDT
Created
attachment 348690
[details]
Patch
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
Created
attachment 348859
[details]
Patch
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
Created
attachment 348864
[details]
Patch
Woodrow Wang
Comment 11
2018-09-04 16:33:21 PDT
Created
attachment 348871
[details]
Patch
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
Created
attachment 348877
[details]
Patch
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
Created
attachment 348930
[details]
Patch
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
Created
attachment 348949
[details]
Patch
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
<
rdar://problem/44154196
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug