Bug 147050 – [Content Extensions] Cache actions with domains that match everything

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

If you have lots of rules with triggers like {"url-filter":".*","if-domain":["*webkit.org"]} we will spend a lot of time adding unnecessary actions to HashSets when you are not on webkit.org.  Caching all the rules and only adding them to a collection once when the domain changes saves a lot of URL interpreting time.

[Alex Christensen]

Created attachment 256991 [details]
Patch

[Alex Christensen]

Created attachment 256996 [details]
Patch

[Alex Christensen]

Created attachment 256998 [details]
Patch

[Benjamin Poulain]

Comment on attachment 256998 [details]
Patch

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

I really like your approach in this patch.

I want to double-check the results on device. I am surprised it does not help the runtime more.

> Source/WebCore/contentextensions/ContentExtension.cpp:88
> +            css.append(action.stringArgument());

While not strictly equivalent, it would be better to merge all the selectors and let the style subsystem split them as needed.

> Source/WebCore/contentextensions/ContentExtension.cpp:98
> +    // These actions don't need to be applied individually any more. They will all be applied to every page as a precompiled style sheet.
> +    m_universalActionsWithoutDomains.removeAllMatching(inGlobalDisplayNoneStyleSheet);

All of the computation above should be done when populating m_universalActionsWithoutDomains. Otherwise there is a window between the constructor and ContentExtension::globalDisplayNoneStyleSheet() during which m_universalActionsWithoutDomains has the CSS rules.

After removing all the CSS, you should do m_universalActionsWithoutDomains.shrinkToFit(). That vector will never need to grow again so we can get back as much memory as possible.

> Source/WebCore/contentextensions/ContentExtension.cpp:127
>      }

Could be worth shrinkToFit() both vectors after this.

> Source/WebCore/contentextensions/ContentExtension.cpp:133
> +    ASSERT_WITH_MESSAGE_UNUSED(domain, domain == m_cachedDomain, "universalWithDomains should only be called right after cachedDomainActions with the same domain, so the cache should aways be correct");

This is a bit fragile for the interface.

What about having a new populateCacheIfNeeded(domain) as the first call of universalActionsWithDomains() and cachedDomainActions()?

> Source/WebCore/contentextensions/ContentExtensionCompiler.cpp:-270
> -                if (!ignorePreviousRulesSeen

You can completely remove "ignorePreviousRulesSeen" without this condition.

> Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:184
> +                if (instruction == DFABytecodeInstruction::AppendAction)
> +                    programCounter += instructionSizeWithArguments(DFABytecodeInstruction::AppendAction);
> +                else if (instruction == DFABytecodeInstruction::AppendActionWithIfDomain)
> +                    programCounter += instructionSizeWithArguments(DFABytecodeInstruction::AppendActionWithIfDomain);

It is unfortunate we still need to touch all this memory. The load-store unit is out next bottleneck.

Can you find out how much memory load and skip at the beginning of machines?

[Alex Christensen]

(In reply to comment #4)
> All of the computation above should be done when populating
> m_universalActionsWithoutDomains. Otherwise there is a window between the
> constructor and ContentExtension::globalDisplayNoneStyleSheet() during which
> m_universalActionsWithoutDomains has the CSS rules.
This was intentional to keep the load time low, but we have to do it anyway before loading a page.  I think I'll change this.
> What about having a new populateCacheIfNeeded(domain) as the first call of
> universalActionsWithDomains() and cachedDomainActions()?
This would make an unnecessary string copy of a short string, but I think the stability would be worth it.
> You can completely remove "ignorePreviousRulesSeen" without this condition.
Right.
> Can you find out how much memory load and skip at the beginning of machines?
We could move all these actions into separate "vectors" in the file and never touch it again.

[Alex Christensen]

Comment on attachment 256998 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtension.cpp:50
> +    copyToVector(withoutDomains.actionsMatchingEverything(), m_universalActionsWithoutDomains);

iOS doesn't like this because it's implicitly casting uint64_t's to uint32_t's.  I need to make this into a for loop with an assertion that the casting doesn't lose any information.

[Alex Christensen]

Created attachment 257136 [details]
Patch

[Alex Christensen]

Created attachment 257137 [details]
Patch

[Benjamin Poulain]

Comment on attachment 257137 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtension.cpp:107
> +    css.append(ContentExtensionsBackend::displayNoneCSSRule());

What a waste of CPU cycles :(

> Source/WebCore/contentextensions/ContentExtension.cpp:136
> +        

shrinkToFit() both vectors?

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:136
> -    WTFLogAlways("Time added: %f microseconds %s", (addedTimeEnd - addedTimeStart) * 1.0e6, resourceLoadInfo.resourceURL.string().utf8().data());
> +    dataLogF("Time added: %f microseconds %s \n", (addedTimeEnd - addedTimeStart) * 1.0e6, resourceLoadInfo.resourceURL.string().utf8().data());

Am I the only one having trouble with dataLog on device?

[Alex Christensen]

Comment on attachment 257137 [details]
Patch

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

>> Source/WebCore/contentextensions/ContentExtension.cpp:136
>> +        
> 
> shrinkToFit() both vectors?

good idea!

>> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:136
>> +    dataLogF("Time added: %f microseconds %s \n", (addedTimeEnd - addedTimeStart) * 1.0e6, resourceLoadInfo.resourceURL.string().utf8().data());
> 
> Am I the only one having trouble with dataLog on device?

no.  I just reroute dataLogF to WTFLogAlways when debugging on device.  There's probably a better way to do it.

[Alex Christensen]

http://trac.webkit.org/changeset/187049

[Alex Christensen]

Got a little too ambitious with that shrinking and committed build fix to http://trac.webkit.org/changeset/187051

[Radar WebKit Bug Importer]

<rdar://problem/21913164>