Bug 144010 – Content extension with oft-repeated rules can cause slowdown

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Brady Eidson]

Content extension with oft-repeated rules can cause slowdown.

With a non-trivial extension adding multiple CSS rules per sub resource, we can't afford to create a new stylesheet each time, especially when they can often represent exactly the same rulesets.

We should have a single stylesheet per document and track which action-ids have added a display:none selector so we don't duplicate anything.

<rdar://problem/20618511>

[Brady Eidson]

Created attachment 251269 [details]
Patch v1

[Alex Christensen]

Comment on attachment 251269 [details]
Patch v1

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

Thanks Brady! We desperately need this, but I'm in a crunch right now.  r- even though this is very close to what we need.  I just think the Actions should be handled a little differently.

> Source/WebCore/ChangeLog:3
> +        Need a short description (OOPS!).

Describe.

> Source/WebCore/ChangeLog:8
> +        No new tests (No behavior change, behavior covered by existing tests).

I wouldn't say "No behavior change", but this is covered by existing tests.

> Source/WebCore/contentextensions/ContentExtensionRule.h:59
> +        , m_actionID(UINT64_MAX)

std::numeric_limits<unsigned>::max()

> Source/WebCore/contentextensions/ContentExtensionRule.h:91
> +    void setActionID(uint64_t actionID) { m_actionID = actionID; }

You could probably get rid of this and setExtensionIdentifier and have the location be an additional constructor parameter once you change Action::deserialize (see later comment)

> Source/WebCore/contentextensions/ContentExtensionRule.h:96
> +    String m_extensionIdentifier;

I'm quite opposed to adding a String here.  We need to change the return type of ContentExtensionsBackend::actionsForResourceLoad to return something like a Vector<std::pair<String, Vector<Action>>>.  Actually, it should probably be a Vector<ActionsForResourceLoad> because we should put more things in a class there and not here.

> Source/WebCore/contentextensions/ContentExtensionRule.h:98
> +    uint64_t m_actionID;

unsigned m_location.
We should use the location (in the serialized actions from Action::deserialize).

> Source/WebCore/contentextensions/ContentExtensionStyleSheet.cpp:47
> +    ASSERT(selectorID && selectorID != UINT64_MAX);

location can be 0.

> Source/WebCore/contentextensions/ContentExtensionStyleSheet.h:57
> +    HashSet<uint64_t> m_addedSelectorIDs;

HashSet<uint64_t, DefaultHash<uint64_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> because location can be 0.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:109
> +                action.setActionID(i);

This should be part of Action::deserialize.

[Brady Eidson]

We discussed this in person, followup patch on its way.

[Darin Adler]

Comment on attachment 251269 [details]
Patch v1

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

>> Source/WebCore/contentextensions/ContentExtensionRule.h:59
>> +        , m_actionID(UINT64_MAX)
> 
> std::numeric_limits<unsigned>::max()

std::numeric_limits<uint64_t>::max()

[Alex Christensen]

(In reply to comment #4)
> Comment on attachment 251269 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251269&action=review
> 
> >> Source/WebCore/contentextensions/ContentExtensionRule.h:59
> >> +        , m_actionID(UINT64_MAX)
> > 
> > std::numeric_limits<unsigned>::max()
> 
> std::numeric_limits<uint64_t>::max()

I also wanted to change the type to unsigned :)

[Brady Eidson]

Created attachment 251275 [details]
Patch v2

[Alex Christensen]

Comment on attachment 251275 [details]
Patch v2

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

> Source/WebCore/contentextensions/ContentExtensionRule.h:95
> +    String m_extensionIdentifier;

I don't want this here, but we can clean this up later.

[Build Bot]

Comment on attachment 251275 [details]
Patch v2

Attachment 251275 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6077211144617984

New failing tests:
http/tests/contentextensions/css-display-none.html

[Build Bot]

Created attachment 251286 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brady Eidson]

Definitely was not failing locally.

Will take a look in the A.M.

[Brady Eidson]

Running run-webkit-tests locally I cannot reproduce this.

But running WKTR on this one test directly, I can reliably reproduce.

This is almost certainly timing related.

[Brady Eidson]

Created attachment 251381 [details]
Patch for landing

[Brady Eidson]

Created attachment 251384 [details]
Patch for landing v2

[Brady Eidson]

Created attachment 251385 [details]
Patch for landing v3

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 251269 [details]

> > Source/WebCore/contentextensions/ContentExtensionRule.h:98
> > +    uint64_t m_actionID;
> 
> unsigned m_location.

I thought I'd made it 64 bit for a reason, then the build error on iOS reminded me:
/Volumes/Data/EWS/WebKit/Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:119:77: error: implicit conversion loses integer precision: 'unsigned long long' to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
                Action action = Action::deserialize(actions, actionsLength, actionLocation);

Because while some of the action locations are 32 bits, when we pull from a:
HashSet<uint64_t, DefaultHash<uint64_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>>
... the actions are 64 bits.

I'm going to go back to 64 bits for landing, then if you later determine that action location can, in fact, be 32 bits we can take it all down to 32.

[Brady Eidson]

Created attachment 251438 [details]
Patch for landing v4

[Brady Eidson]

Created attachment 251443 [details]
Patch for landing v5

[Brady Eidson]

(In reply to comment #15)
> 
> I'm going to go back to 64 bits for landing, then if you later determine
> that action location can, in fact, be 32 bits we can take it all down to 32.

NM, I see what I'd changed that caused this. Back to 32...

[Brady Eidson]

Created attachment 251446 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 251446 [details]
Patch for landing

Clearing flags on attachment: 251446

Committed r183195: <http://trac.webkit.org/changeset/183195>