Bug 189213

Summary: Add infrastructure to dump resource load statistics
Product: WebKit Reporter: Woodrow Wang <wwang153>
Component: New BugsAssignee: Woodrow Wang <wwang153>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, dbates, ews-watchlist, japhet, pvollan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 187773    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews203 for win-future
none
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch none

Description Woodrow Wang 2018-08-31 15:31:51 PDT
Added dumping function for testing resource load statistics
Comment 1 Woodrow Wang 2018-08-31 15:43:57 PDT
Created attachment 348679 [details]
Patch
Comment 2 Woodrow Wang 2018-08-31 17:07:03 PDT
Created attachment 348683 [details]
Patch
Comment 3 Woodrow Wang 2018-08-31 17:40:21 PDT
Created attachment 348690 [details]
Patch
Comment 4 Daniel Bates 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?
Comment 5 Woodrow Wang 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.
Comment 6 Woodrow Wang 2018-09-04 15:41:25 PDT
Created attachment 348859 [details]
Patch
Comment 7 Chris Dumez 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.
Comment 8 Daniel Bates 2018-09-04 16:09:52 PDT
Comment on attachment 348859 [details]
Patch

r- per comment #7.
Comment 9 Woodrow Wang 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.
Comment 10 Woodrow Wang 2018-09-04 16:18:55 PDT
Created attachment 348864 [details]
Patch
Comment 11 Woodrow Wang 2018-09-04 16:33:21 PDT
Created attachment 348871 [details]
Patch
Comment 12 Chris Dumez 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?
Comment 13 Woodrow Wang 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.
Comment 14 Woodrow Wang 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?
Comment 15 Woodrow Wang 2018-09-04 17:27:38 PDT
Created attachment 348877 [details]
Patch
Comment 16 Chris Dumez 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?
Comment 17 EWS Watchlist 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
Comment 18 EWS Watchlist 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
Comment 19 Chris Dumez 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.
Comment 20 Woodrow Wang 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.
Comment 21 Woodrow Wang 2018-09-05 09:30:57 PDT
Created attachment 348930 [details]
Patch
Comment 22 Chris Dumez 2018-09-05 09:42:11 PDT
Comment on attachment 348930 [details]
Patch

LGTM
Comment 23 Chris Dumez 2018-09-05 09:56:25 PDT
(In reply to Chris Dumez from comment #22)
> Comment on attachment 348930 [details]
> Patch
> 
> LGTM

r=me
Comment 24 EWS Watchlist 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
Comment 25 EWS Watchlist 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
Comment 26 Woodrow Wang 2018-09-05 11:28:30 PDT
Created attachment 348949 [details]
Patch
Comment 27 WebKit Commit Bot 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>
Comment 28 WebKit Commit Bot 2018-09-05 14:25:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Radar WebKit Bug Importer 2018-09-05 14:27:29 PDT
<rdar://problem/44154196>