Bug 155242 – Move prevalent resource classifier from WebCore to WebKit

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

Move prevalent resource classifier from WebCore to WebKit after the WebKit2 implementation.

[John Wilander]

rdar://problem/24913272

[John Wilander]

Created attachment 273445 [details]
Patch

[WebKit Commit Bot]

Attachment 273445 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 273462 [details]
Patch

[John Wilander]

Filed https://bugs.webkit.org/show_bug.cgi?id=155243 regarding the style errors.

[WebKit Commit Bot]

Attachment 273462 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 273464 [details]
Patch

[WebKit Commit Bot]

Attachment 273464 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

The classifier needs to include StringHash.h except for on debug builds.

[Brent Fulgham]

Comment on attachment 273464 [details]
Patch

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

This is looking pretty good, but it needs a bit more work.

> Source/WebCore/ChangeLog:4
> +        rdar://problem/24913272

We write this as <rdar://problem/24913272>

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=155242

Bugzilla bug goes BEFORE the corresponding radar. Most other people don't care about the Radar number! :-)

> Source/WebCore/ChangeLog:24
> [phone]  John Wilander  <[e-mail]>

Whoops! It looks like you accidentally ran "prepare-ChangeLog" twice. Please delete the duplicate data below.

> Source/WebKit2/ChangeLog:4
> +        rdar://problem/24913272

Please put inside <>

> Source/WebKit2/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=155242

Please move above the Radar.

> Source/WebKit2/ChangeLog:18
> [phone]  John Wilander  <[e-mail]>

Please get rid of this duplicate ChangeLog info.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:61
> +    Vector<String> prevalentResourcesWithUserInteraction;

This could just be Vector<String> prevalentResources, prevalentResourcesWithUserInteraction;

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:52
> +        if (resourceStatistic.isPrevalentResource && resourceStatistic.hadUserInteraction) {

We don't use curly braces when there is only one line in the conditional clause.

This might read better as follows:

if (resourceStatistics.isPrevalentResource) {
    if (resourceStatistic.hadUserInteraction)
        prevalentResourcesWithUserInteraction.append(resourceStatistic.highLevelDomain);
    else
        prevalentResources.append(resourceStatistic.highLevelDomain);
    continue;
}

... then you can get rid of the 'else' at line 56.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:55
> +            prevalentResources.append(resourceStatistic.highLevelDomain);

Ditto.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:56
> +        } else if (resourceStatistic.subframeUnderTopFrameOrigins.size() > subframeUnderTopFrameOriginsThreshold

... this could just be:

if (hasPrevalentResourceCharacteristics(resourceStatistics) {
   ... stuff ...
}

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:59
> +                   || resourceStatistic.redirectedToOtherPrevalentResourceOrigins.size() > redirectedToOtherPrevalentResourceOriginsThreshold) {

I liked this much better as a predicate:

static bool hasPrevalentResourceCharacteristics(const ResourceStatistics& ...), like when it was in WebCore. Please change that back.

This is also the cause of your stylebot failures. The "||" should align with one indent below the '}' in the "} else if" statement, not the "if".

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.h:38
> +class WebResourcePrevalenceClassifier {

It doesn't make sense for this to be a class when its only purpose is to be a holder for a single static method.

Instead, I think this should just be a namespace containing one static function.

[Brent Fulgham]

Note: GTK (and probably EFL) is failing because they don't know what the new WebResourcePrevalenceClassifier is:

error: undefined reference to 'WebKit::WebResourcePrevalenceClassifier::classify

Please add it to the the three CMake files where the "UIProcess/WebResourceLoadStatisticsStore.cpp" files are added:

PlatformEfl.cmake:    UIProcess/WebResourceLoadStatisticsStore.cpp
PlatformGTK.cmake:    UIProcess/WebResourceLoadStatisticsStore.cpp
PlatformMac.cmake:    UIProcess/WebResourceLoadStatisticsStore.cpp

[John Wilander]

Created attachment 273470 [details]
Patch

[WebKit Commit Bot]

Attachment 273470 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 273471 [details]
Patch

[WebKit Commit Bot]

Attachment 273471 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:58:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:59:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Wilander]

Created attachment 273473 [details]
Patch

[Brent Fulgham]

Comment on attachment 273473 [details]
Patch

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

You did not correct the ChangeLog per my earlier review comments. r- Until you address those issues.

> Source/WebCore/ChangeLog:4
> +        rdar://problem/24913272

Please place this inside <>.

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=155242

Please move this above the Radar.

> Source/WebKit2/ChangeLog:4
> +        rdar://problem/24913272

Please place this inside <>.

> Source/WebKit2/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=155242

This needs to go above the Radar.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:56
> +        } else if (resourceStatistic.subframeUnderTopFrameOrigins.size() > subframeUnderTopFrameOriginsThreshold

You did not address my request that you make these four lines of "||" into a static predicate method.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.h:38
> +class WebResourcePrevalenceClassifier {

You did not address my comment that a class with a single static method is not the right approach. We are not programming in Java!

This should be a namespace holding this static method.

[Andy Estes]

Comment on attachment 273473 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:33
> +namespace WebKit {
> +class WebResourceLoadStatisticsStore;
> +}

This is a layering violation. WebCore shouldn't know things about WebKit. We need to find another solution.

>> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.h:38
>> +class WebResourcePrevalenceClassifier {
> 
> You did not address my comment that a class with a single static method is not the right approach. We are not programming in Java!
> 
> This should be a namespace holding this static method.

Or just a free function in the WebKit namespace :)

[John Wilander]

Created attachment 273513 [details]
Patch

[John Wilander]

Sorry, Brent! I did not see your comments between my previous patch changes. I was busy fixing the various build errors on the bots.

Andy, the layering violation is still in this patch. I'll find a way to address it.

[Brent Fulgham]

Comment on attachment 273513 [details]
Patch

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

Patch doesn't apply for some reason! Please do an "update-webkit" and try uploading again...

> Source/WebCore/ChangeLog:22
> +           - Made this class friend of WebKit::WebResourceLoadStatisticsStore since the latter needs to send the statistics map for classification.

ResourceLoadStatisticsStore.h has no changes anymore.

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:34
> +

This isn't needed anymore.

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:67
> +    friend class WebKit::WebResourceLoadStatisticsStore;

This isn't needed anymore.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:69
> +bool hasPrevalentResourceCharacteristics(const ResourceLoadStatistics& resourceStatistic)

This should be a static function. Move it above your 'classify' function.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:74
> +    || resourceStatistic.redirectedToOtherPrevalentResourceOrigins.size() > redirectedToOtherPrevalentResourceOriginsThreshold;

Each "||" should be indented another level.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.h:40
> +bool hasPrevalentResourceCharacteristics(const WebCore::ResourceLoadStatistics& resourceStatistic);

This doesn't need to be exposed outside this compilation unit. Just remove this line.

[John Wilander]

Created attachment 273588 [details]
Patch

[John Wilander]

Thanks, Brent! I've addressed all your comments and changed the layering violation to a model where WebKit supplies a classifier lambda for the WebCore part to apply to its statistics map.

[Brent Fulgham]

Comment on attachment 273588 [details]
Patch

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

Looks good! Since this touches WK2 code we need to get Andy to do the final r+. Maybe you could adjust my minor nits and upload a final clean patch for him to review.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:140
> +static const unsigned minimumOriginsLoadedForProcessing = 100;

I recommend putting all of these magic numbers together at the top of the file where they can be easily tweaked.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:148
> +        processFunction(resourceStatistic);

Nice!

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:63
> +    WEBCORE_EXPORT void processStatistics(std::function<void(ResourceLoadStatistics&)>&& processFunction);

You don't need the argument name here.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:65
>      // TODO: Notify individual WebProcesses of prevalent domains. <rdar://problem/24703099>

For completeness, you should mention that this TODO should consume the two lists.

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.h:39
> +void classify(WebCore::ResourceLoadStatistics& resourceStatistic, Vector<String>& prevalentResources, Vector<String>& prevalentResourcesWithUserInteraction);

You can get rid of the "resourceStatistic" argument here, it doesn't provide any new information.

[John Wilander]

Created attachment 273605 [details]
Patch

[John Wilander]

Thanks again, Brent! All those comments addressed.

[Brent Fulgham]

(In reply to comment #26)
> Thanks again, Brent! All those comments addressed.

Great! It looks good. Let's get Andy to do the final review and we can land it.

[alan]

Comment on attachment 273605 [details]
Patch

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

> Source/WebKit2/UIProcess/WebResourcePrevalenceClassifier.cpp:67
> +    if (resourceStatistic.isPrevalentResource) {
> +        if (resourceStatistic.hadUserInteraction)
> +            prevalentResourcesWithUserInteraction.append(resourceStatistic.highLevelDomain);
> +        else
> +            prevalentResources.append(resourceStatistic.highLevelDomain);
> +    } else if (hasPrevalentResourceCharacteristics(resourceStatistic)) {
> +        if (resourceStatistic.hadUserInteraction)
> +            prevalentResourcesWithUserInteraction.append(resourceStatistic.highLevelDomain);
> +        else {
> +            prevalentResources.append(resourceStatistic.highLevelDomain);
> +            resourceStatistic.isPrevalentResource = true;
> +        }
> +    }

I'd rather not duplicate the logic here.

if (resourceStatistic.isPrevalentResource || hasPrevalentResourceCharacteristics(resourceStatistic)) {
    if (resourceStatistic.hadUserInteraction)
        prevalentResourcesWithUserInteraction.append(resourceStatistic.highLevelDomain);
    else {
        prevalentResources.append(resourceStatistic.highLevelDomain);
        resourceStatistic.isPrevalentResource = true
    }
}

[John Wilander]

That means we will set resourceStatistic.isPrevalentResource = true over and over when it is already true but I guess that's OK. Do others agree?

[alan]

(In reply to comment #29)
> That means we will set resourceStatistic.isPrevalentResource = true over and
> over when it is already true but I guess that's OK. Do others agree?
I think it's better than duplicating logic which is highly error prone.

[Brent Fulgham]

(In reply to comment #30)
> (In reply to comment #29)
> > That means we will set resourceStatistic.isPrevalentResource = true over and
> > over when it is already true but I guess that's OK. Do others agree?
> I think it's better than duplicating logic which is highly error prone.

That's a good point. Setting that boolean over and over is very low cost. The multiple branch statements were probably far more expensive!

[Brent Fulgham]

BTW: The iOS build error does not seem to be related to this patch.

[Andy Estes]

Comment on attachment 273605 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:148
> +    if (m_resourceStatisticsMap.size() < minimumOriginsLoadedForProcessing)
> +        return;
> +
> +    for (auto& resourceStatistic : m_resourceStatisticsMap.values())
> +        processFunction(resourceStatistic);

Somewhat unfortunate that WebResourceLoadStatisticsStore has to repeatedly call this function even when there isn't enough data for processing. A better design would be where the processing function is registered up front and then called automatically at the appropriate times.

I'm ok with you landing this as-is, but at some point we should move ResourceLoadStatisticsStore out of WebCore and fold it into WebResourceLoadStatisticsStore. Having this class split between WebCore and WebKit is awkward and no longer seems necessary with WebKit1 out of the picture. In its place could just be an abstract class that ResourceLoadObserver calls into and that is subclassed by something in WebKit. That's the typical pattern we follow for delegating from WebCore to WebKit.

> Source/WebKit2/ChangeLog:18
> +        (WebKit::WebResourcePrevalenceClassifier::hasPrevalentResourceCharacteristics):
> +        (WebKit::WebResourcePrevalenceClassifier::classify):
> +        * UIProcess/WebResourcePrevalenceClassifier.h: Added.
> +           - Added new classifier for prevalent subresources.

A new .cpp file and namespace seem like overkill just to add a function that is only called in one place. Why not just define classify() and its helpers as static functions in WebResourceLoadStatisticsStore.cpp?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:65
> +    // TODO: Notify individual WebProcesses of prevalent domains using the two vectors populated by the classifier. <rdar://problem/24703099>

WebKit style says to use FIXME instead of TODO.

[John Wilander]

Zalan, your comment actually caught a subtle bug. This is how it should look:
if (resourceStatistic.isPrevalentResource || hasPrevalentResourceCharacteristics(resourceStatistic)) {
    resourceStatistic.isPrevalentResource = true;
    if (resourceStatistic.hadUserInteraction)
        prevalentResourcesWithUserInteraction.append(resourceStatistic.highLevelDomain);
    else
        prevalentResources.append(resourceStatistic.highLevelDomain);
}

Andy, I like your comments. I've been onto this split of where the code resides but haven't really been able to suggest the right change. Anyhow, I'll gate the lamda call on an initial check and also get rid of the classifier files by moving the code to WebResourceLoadStatisticsStore.

New patch coming up.

Thanks!

[John Wilander]

Created attachment 273668 [details]
Patch

[Andy Estes]

Comment on attachment 273668 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:149
> +    if (hasEnoughDataForStatisticsProcessing()) {

You only call processStatistics() when there's enough data, so I'd change this if statement to ASSERT(hasEnoughDataForStatisticsProcessing()). No need to check twice.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:179
> +bool WebResourceLoadStatisticsStore::hasPrevalentResourceCharacteristics(const ResourceLoadStatistics& resourceStatistic)

This function doesn't access an instance of WebResourceLoadStatisticsStore, so it shouldn't be a member function. It should be a static inline free function:

    static inline bool hasPrevalentResourceCharacteristics(const ResourceLoadStatistics&) { ... }

You'll want to move the definition above the first call site to avoid having to forward-declare.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:184
> +    return resourceStatistic.subframeUnderTopFrameOrigins.size() > subframeUnderTopFrameOriginsThreshold
> +    || resourceStatistic.subresourceUnderTopFrameOrigins.size() > subresourceUnderTopFrameOriginsThreshold
> +    || resourceStatistic.subresourceUniqueRedirectsTo.size() > subresourceHasBeenRedirectedFromToUniqueDomainsThreshold
> +    || resourceStatistic.redirectedToOtherPrevalentResourceOrigins.size() > redirectedToOtherPrevalentResourceOriginsThreshold;

Be sure to re-indent this.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:187
> +void WebResourceLoadStatisticsStore::classifyPrevalentResources(ResourceLoadStatistics& resourceStatistic, Vector<String>& prevalentResources, Vector<String>& prevalentResourcesWithUserInteraction)

Previous comment about member functions applies here too.

[John Wilander]

Created attachment 273776 [details]
Patch

[John Wilander]

Thanks, Andy! All your comments covered by the new patch.

[Andy Estes]

Comment on attachment 273776 [details]
Patch

Looks good, thanks!

FYI: I r+'d the previous patch, so it would've been ok for you to just land your change directly after addressing the feedback.

[WebKit Commit Bot]

Comment on attachment 273776 [details]
Patch

Clearing flags on attachment: 273776

Committed r198055: <http://trac.webkit.org/changeset/198055>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.