WIP: We should resurrect the older patch that collects some statistics of web API calls
Created attachment 402162 [details] Patch
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.
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 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.
(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.
Created attachment 402290 [details] Patch
Created attachment 402331 [details] Patch
Created attachment 402361 [details] Patch
Created attachment 402375 [details] Patch
Created attachment 402477 [details] Patch
You should ask a JSC engineer about the failing test.
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?
(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.
(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.
Created attachment 402599 [details] Patch
Created attachment 402612 [details] Patch
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 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 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 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.
Committed r263474: <https://trac.webkit.org/changeset/263474> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402612 [details].
<rdar://problem/64731014>