Bug 213319 - We should resurrect the older patch that collects some statistics of web API calls
Summary: We should resurrect the older patch that collects some statistics of web API ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-17 15:50 PDT by Umar Iqbal
Modified: 2020-06-24 17:45 PDT (History)
15 users (show)

See Also:


Attachments
Patch (12.70 KB, patch)
2020-06-17 15:52 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (20.10 KB, patch)
2020-06-19 09:15 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2020-06-19 15:06 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (23.35 KB, patch)
2020-06-19 17:35 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (25.25 KB, patch)
2020-06-19 21:44 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (35.31 KB, patch)
2020-06-22 08:35 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (48.73 KB, patch)
2020-06-23 15:45 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff
Patch (41.03 KB, patch)
2020-06-23 17:36 PDT, Umar Iqbal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Umar Iqbal 2020-06-17 15:50:52 PDT
WIP: We should resurrect the older patch that collects some statistics of web API calls
Comment 1 Umar Iqbal 2020-06-17 15:52:54 PDT
Created attachment 402162 [details]
Patch
Comment 2 John Wilander 2020-06-17 16:12:50 PDT
Comment on attachment 402162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402162&action=review

Great to see some code! We should make sure to enable the associated tests which will probably involve touching the test expectation files.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

As long as we make sure that the associated tests are run you can change this to "No new tests. Enabled existing tests."

> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet)

Was this something that was removed earlier? I'm surprised this is needed.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:77
> +

Even if we find whitespace errors, we try not to change them unless we are changing the code adjacent to it. Otherwise it makes the revision history hard to read because it looks like you changed something here, for instance deleted code, where in fact you just removed whitespace.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);

Oh, so this was the reason for the addition of the encodeHashSet() function? This is something you want to call out in the change log – both that you did it and why you did it. "Changed encodeHashCountedSet() to encodeHashSet() because …"

I'm curious, did you change from counted set to just set?

> Source/WebCore/loader/ResourceLoadStatistics.cpp:198
> +IGNORE_WARNINGS_BEGIN("unused-result")

Yeah, this is a result of our sparse storage. We decided early on to not store empty data structures which means that an empty return in decoding should not be interpreted as an error but an empty (sub)structure.

> Source/WebCore/loader/ResourceLoadStatistics.h:126
> +    HashSet<RegistrableDomain> topFrameRegistrableDomainsWhichAccessedWebAPIs;

Interesting! We apparently introduced RegistrableDomain after this code was landed.
Comment 3 katherine_cheney 2020-06-18 10:13:09 PDT
Yay, first patch! Before landing, I think you should probably enable http/tests/webAPIStatistics tests and re-upload to confirm tests are passing on EWS.
Comment 4 Umar Iqbal 2020-06-18 19:12:36 PDT
Comment on attachment 402162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402162&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests (OOPS!).
> 
> As long as we make sure that the associated tests are run you can change this to "No new tests. Enabled existing tests."

I will enable the http/tests/webAPIStatistics tests and re-upload along with the specified message.

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
>> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet)
> 
> Was this something that was removed earlier? I'm surprised this is needed.

I brought it back because it was needed by encodeFontHashSet which becomes functional by enabling WEB_API_STATISTICS

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:77
>> +
> 
> Even if we find whitespace errors, we try not to change them unless we are changing the code adjacent to it. Otherwise it makes the revision history hard to read because it looks like you changed something here, for instance deleted code, where in fact you just removed whitespace.

I will update accordingly.

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
>> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);
> 
> Oh, so this was the reason for the addition of the encodeHashSet() function? This is something you want to call out in the change log – both that you did it and why you did it. "Changed encodeHashCountedSet() to encodeHashSet() because …"
> 
> I'm curious, did you change from counted set to just set?

I changed from counted hash set to hash set because in one of your earlier patch (https://bugs.webkit.org/attachment.cgi?id=363033) you mentioned that the count was never used. So I followed the same principle and updated to hash set.
Comment 5 Umar Iqbal 2020-06-18 19:14:25 PDT
(In reply to katherine_cheney from comment #3)
> Yay, first patch! Before landing, I think you should probably enable
> http/tests/webAPIStatistics tests and re-upload to confirm tests are passing
> on EWS.

Yes! I will update accordingly.
Comment 6 Umar Iqbal 2020-06-19 09:15:09 PDT
Created attachment 402290 [details]
Patch
Comment 7 Umar Iqbal 2020-06-19 15:06:20 PDT
Created attachment 402331 [details]
Patch
Comment 8 Umar Iqbal 2020-06-19 17:35:25 PDT
Created attachment 402361 [details]
Patch
Comment 9 Umar Iqbal 2020-06-19 21:44:44 PDT
Created attachment 402375 [details]
Patch
Comment 10 Umar Iqbal 2020-06-22 08:35:56 PDT
Created attachment 402477 [details]
Patch
Comment 11 John Wilander 2020-06-22 19:43:59 PDT
You should ask a JSC engineer about the failing test.
Comment 12 John Wilander 2020-06-22 19:52:49 PDT
Comment on attachment 402477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402477&action=review

The code looks good but there's a JSC test failure and change log entries are missing for Layout Tests, Tools, JSC, WTF, and PAL (I think, even though it's under WebCore). The prepare-Changelog script should have added them.

> Source/WebCore/ChangeLog:3
> +        WIP: We should resurrect the older patch that collects some statistics of web API calls

Is this patch a work-in-progress or ready for landing?

> Source/WebCore/ChangeLog:8
> +        No new tests. Enabled existing tests. Enabled http/tests/webAPIStatistics that test the functionality behind WEB_API_STATISTICS flag.

Excellent to see these tests re-enabled! Look at previous change log entries for the approximate line width. These long lines make it hard to read.

> Source/WebCore/ChangeLog:9
> +        + Brought back encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet) because it was needed by encodeFontHashSet(KeyedEncoder& encoder, const String& label, const HashSet<String>& hashSet)

When referring to functions in WebCore or WebKit, please include the name space.

> Source/WebCore/loader/ResourceLoadStatistics.h:105
> +    enum class NavigatorAPI : uint8_t {

Do we have a trace of why these were 64 before?
Comment 13 Umar Iqbal 2020-06-23 10:35:01 PDT
(In reply to John Wilander from comment #11)
> You should ask a JSC engineer about the failing test.

Checked with Keith Miller. He said that the test is flaky and it's just best effort on jsc-mips.
Comment 14 Keith Miller 2020-06-23 13:03:24 PDT
(In reply to Umar Iqbal from comment #13)
> (In reply to John Wilander from comment #11)
> > You should ask a JSC engineer about the failing test.
> 
> Checked with Keith Miller. He said that the test is flaky and it's just best
> effort on jsc-mips.

To clarify, it's more that Apple folks try to keep the jsc-mips bots green but we don't maintain them. In this case it's almost impossible this patch broke anything.
Comment 15 Umar Iqbal 2020-06-23 15:45:39 PDT
Created attachment 402599 [details]
Patch
Comment 16 Umar Iqbal 2020-06-23 17:36:25 PDT
Created attachment 402612 [details]
Patch
Comment 17 Umar Iqbal 2020-06-24 07:41:37 PDT
Comment on attachment 402477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402477&action=review


>> The code looks good but there's a JSC test failure and change log entries are missing for Layout Tests, Tools, JSC, WTF, and PAL (I think, even though it's under WebCore). The prepare-Changelog script should have added them.

The new patch now has the change log for  Layout Tests, Tools, JSC, WTF, and PAL 


>> Source/WebCore/ChangeLog:3
>> +        WIP: We should resurrect the older patch that collects some statistics of web API calls
> 
> Is this patch a work-in-progress or ready for landing?

I have updated the title to reflect that.

>> Source/WebCore/ChangeLog:8
>> +        No new tests. Enabled existing tests. Enabled http/tests/webAPIStatistics that test the functionality behind WEB_API_STATISTICS flag.
> 
> Excellent to see these tests re-enabled! Look at previous change log entries for the approximate line width. These long lines make it hard to read.

I have reduced the line width in change log.

>> Source/WebCore/ChangeLog:9
>> +        + Brought back encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet) because it was needed by encodeFontHashSet(KeyedEncoder& encoder, const String& label, const HashSet<String>& hashSet)
> 
> When referring to functions in WebCore or WebKit, please include the name space.

Done.

>> Source/WebCore/loader/ResourceLoadStatistics.h:105
>> +    enum class NavigatorAPI : uint8_t {
> 
> Do we have a trace of why these were 64 before?

As discussed on Slack. I have reverted it back to uint64_t. I will revert it to uint8_t in a separate patch.
Comment 18 Brent Fulgham 2020-06-24 10:17:14 PDT
Comment on attachment 402612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402612&action=review

I think this is headed in the right direction, but seems to re-introduce code that uses Strings instead of RegistrableDomains, which is not what we want. I also don't understand the reason to switch the existing code from HashCountedSet to HashSet, and would like some explanation of that.

Finally, because this could impact performance, I do not support turning it on by default, everywhere as this current patch does. We should have a runtime flag to turn it on/off that can be used for testing before we enabled this to compile and run at all times.

> Source/WebKitLegacy/mac/ChangeLog:8
> +        + Enabled ENABLE_WEB_API_STATISTICS flag

Do we actually want this turned on for all users, all the time? I would expect this to either be controlled by a debug menu flag, or for this to be off everywhere and only turned on when building to work on this feature. I do not support turning this on by default yet.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet)

This method appears to already exist in my code, outside of this ENABLE() block. Indeed, if I look up a few dozen lines in this tool I can see the official 'encodeHashSet' method. It is also based on the correct, modern RegistrableDomain code. I think you can entirely delete this new function.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);

Can you clarify why you are changing from a HashCounted set to a HashSet?

> Source/WebCore/loader/ResourceLoadStatistics.cpp:177
> +static void decodeHashSet(KeyedDecoder& decoder, const String& label, const String& key, HashSet<String>& hashSet)

Likewise, this already exists but uses RegistrableDomain instead of String. I don't think we want to use this code.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:342
> +        decodeHashSet(decoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);

Ditto regarding change from HashCounted to Hash

> Source/WebCore/loader/ResourceLoadStatistics.cpp:377
> +static void appendHashSet(StringBuilder& builder, const String& label, const HashSet<String>& hashSet)

Why is this needed, when there is an existing 'appendHashSet' that uses RegistrableDomain?

> LayoutTests/platform/ios-wk2/TestExpectations:72
> +http/tests/webAPIStatistics [ Pass ]

I would leave this Skipped until we have a toggle to turn the feature on/off dynamically.

> LayoutTests/platform/mac-wk2/TestExpectations:73
> +http/tests/webAPIStatistics [ Pass ]

Ditto.
Comment 19 Umar Iqbal 2020-06-24 12:32:22 PDT
Comment on attachment 402612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402612&action=review

>> Source/WebKitLegacy/mac/ChangeLog:8
>> +        + Enabled ENABLE_WEB_API_STATISTICS flag
> 
> Do we actually want this turned on for all users, all the time? I would expect this to either be controlled by a debug menu flag, or for this to be off everywhere and only turned on when building to work on this feature. I do not support turning this on by default yet.

We also have a runtime flag to turn it on/off from the settings menu (in internal features) and it is set to false in RuntimeEnabledFeatures.h by default.

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
>> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet)
> 
> This method appears to already exist in my code, outside of this ENABLE() block. Indeed, if I look up a few dozen lines in this tool I can see the official 'encodeHashSet' method. It is also based on the correct, modern RegistrableDomain code. I think you can entirely delete this new function.

It is needed by WebCore::ResourceLoadStatistics::fontsFailedToLoad which stores font family as String. Since RegistrableDomains is designed to store domains, and not font family, we should keep it to encode string HashSet used by fontsFailedToLoad.

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
>> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);
> 
> Can you clarify why you are changing from a HashCounted set to a HashSet?

In an earlier patch (https://bugs.webkit.org/show_bug.cgi?id=195071, check the changelog) it was mentioned that the counts were never used and the patch changed the other HashCountedSets to HashSets (e.g. WebCore::ResourceLoadStatistics::topFrameUniqueRedirectsTo). I followed the same principle and updated HashCountedSets to HashSet for WebCore::ResourceLoadStatistics::topFrameRegistrableDomainsWhichAccessedWebAPIs

>> Source/WebCore/loader/ResourceLoadStatistics.cpp:177
>> +static void decodeHashSet(KeyedDecoder& decoder, const String& label, const String& key, HashSet<String>& hashSet)
> 
> Likewise, this already exists but uses RegistrableDomain instead of String. I don't think we want to use this code.

Same reason as explained earlier for encodeHashSet

>> LayoutTests/platform/ios-wk2/TestExpectations:72
>> +http/tests/webAPIStatistics [ Pass ]
> 
> I would leave this Skipped until we have a toggle to turn the feature on/off dynamically.

Since we turn the feature on/off from the settings menu. We can keep it.
Comment 20 Brent Fulgham 2020-06-24 13:26:33 PDT
Comment on attachment 402612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402612&action=review

okay, looks good!

>>> Source/WebKitLegacy/mac/ChangeLog:8
>>> +        + Enabled ENABLE_WEB_API_STATISTICS flag
>> 
>> Do we actually want this turned on for all users, all the time? I would expect this to either be controlled by a debug menu flag, or for this to be off everywhere and only turned on when building to work on this feature. I do not support turning this on by default yet.
> 
> We also have a runtime flag to turn it on/off from the settings menu (in internal features) and it is set to false in RuntimeEnabledFeatures.h by default.

Okay, good.

>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
>>> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const String& key, const HashSet<String>& hashSet)
>> 
>> This method appears to already exist in my code, outside of this ENABLE() block. Indeed, if I look up a few dozen lines in this tool I can see the official 'encodeHashSet' method. It is also based on the correct, modern RegistrableDomain code. I think you can entirely delete this new function.
> 
> It is needed by WebCore::ResourceLoadStatistics::fontsFailedToLoad which stores font family as String. Since RegistrableDomains is designed to store domains, and not font family, we should keep it to encode string HashSet used by fontsFailedToLoad.

Got it. Thanks for clarifying.

>>> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
>>> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);
>> 
>> Can you clarify why you are changing from a HashCounted set to a HashSet?
> 
> In an earlier patch (https://bugs.webkit.org/show_bug.cgi?id=195071, check the changelog) it was mentioned that the counts were never used and the patch changed the other HashCountedSets to HashSets (e.g. WebCore::ResourceLoadStatistics::topFrameUniqueRedirectsTo). I followed the same principle and updated HashCountedSets to HashSet for WebCore::ResourceLoadStatistics::topFrameRegistrableDomainsWhichAccessedWebAPIs

Seems reasonable.

>>> LayoutTests/platform/ios-wk2/TestExpectations:72
>>> +http/tests/webAPIStatistics [ Pass ]
>> 
>> I would leave this Skipped until we have a toggle to turn the feature on/off dynamically.
> 
> Since we turn the feature on/off from the settings menu. We can keep it.

Agreed.
Comment 21 EWS 2020-06-24 14:18:16 PDT
Committed r263474: <https://trac.webkit.org/changeset/263474>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402612 [details].
Comment 22 Radar WebKit Bug Importer 2020-06-24 17:45:57 PDT
<rdar://problem/64731014>