[LazyParseCSS] Ensure UseCounting has parity with strict parsing

[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722
Committed: https://crrev.com/f8090ce1eb0631947f38a75bd27fa8325facf921
Cr-Commit-Position: refs/heads/master@{#436439}

[Charlie Harrison, 2016-11-01 21:39:07]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 21:40:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-11-01 21:45:18]

Description was changed from

==========
[LazyParseCSS] Cache the Document as a proxy for UseCounter changes.

For lazy parsing, we need to be careful to update the parsing context's
UseCounter the owning Document swaps out from under us. This patch changes
that logic from using UseCounter::getFrom to using the StyleSheetContents
singleOwnerDocument.

Using UseCounter::getFrom was costing ~5.5% overhead when all the rules are
parsed. Moving to singleOwnerDocument moves this to 2.5% overhead.

BUG=642722
==========

to

==========
[LazyParseCSS] Cache the Document as a proxy for UseCounter changes.

For lazy parsing, we need to be careful to update the parsing context's
UseCounter the owning Document swaps out from under us. This patch changes
that logic from using UseCounter::getFrom to using the StyleSheetContents
singleOwnerDocument.

Using UseCounter::getFrom was costing ~4.5% overhead when all the rules are
parsed. Moving to singleOwnerDocument moves this to ~2% overhead.

BUG=642722
==========

[Charlie Harrison, 2016-11-01 21:52:42]

csharrison@chromium.org changed reviewers:
+ timloh@chromium.org

[Charlie Harrison, 2016-11-01 21:52:42]

timloh: PTAL at this quick fix? I think semantics should be identical.

If there is a more efficient way to get at this data without invading too much
into StyleSheetContents please let me know, as singleOwnerDocument() isn't free.

[commit-bot: I haz the power, 2016-11-02 00:12:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 00:12:18]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-21 03:07:34]

timloh: friendly ping.

[Timothy Loh, 2016-11-30 14:58:26]

On 2016/11/21 03:07:34, Charlie Harrison wrote:
> timloh: friendly ping.

I'm not really familiar with StyleSheetContents -- do you know what the
sheetContents->hasSingleOwnerNode() call on UseCounter::getFrom() is for? Looks
like we're no longer calling it.

[Charlie Harrison, 2016-11-30 15:16:10]

We still call it when we end up updating the parsing context (which we do at
least once). I think that check is for cases where a single StyleSheetContents
is used by multiple CSSStyleSheets within a single document.

We don't want to count in shared cases, but if the first call to parse sees an
unshared contents, I think it is reasonable for us to use the counter for the
rest of parsing, even if the sheet ends up being shared within the document
later. This is the current behavior with the experiment off, I believe (because
we do all the parsing right away before the contents is shared).

That being said, we should maybe rope in a UseCounter expert to verify.

[Charlie Harrison, 2016-11-30 19:53:49]

csharrison@chromium.org changed reviewers:
+ rune@opera.com

[Charlie Harrison, 2016-11-30 19:53:49]

+rune for a bit of help with StyleSheetContents and UseCounters.

[rune, 2016-11-30 20:47:59]

On 2016/11/30 19:53:49, Charlie Harrison wrote:
> +rune for a bit of help with StyleSheetContents and UseCounters.

We have two levels of StyleSheetContents caching. One is a cache for parsing
<style> elements inside the same document (through parseSheet in StyleEngine
IIRC). I think the caching here mainly is for shadow dom where you can have many
components with the same stylesheet. These sheets may even share the
StyleSheetContents containing media queries as the instances are always
evaluated against the same media features as they are present in the same
document. For this cache I believe you have a non-null singleOwnerDocument().

The other caching is at the resource level where you have link rel=stylesheet
and a StyleSheetContents which is shared between links to the same resource.
Here we share the parsed StyleSheetContents potentially between multiple
documents from different domains, even. If there are multiple "clients" in a
single document only, singleOwnerDocument() will return a non-null pointer. At
the point where multiple documents reference the StyleSheetContents,
singleOwnerDocument() will return null as m_hasSingleOwnerDocument will have
been set to false.

Now, even before LazyParsing, the use counters are typically counted wrong when
shared as the sheet will be parsed once, triggering use counters once. Imagine
every site in the world referencing the same boilerplate sheet from the same
cdn. It will be counted once, even though it's used by every site.

Without lazy parsing, the use counter will be the one associated with the
document first referencing the stylesheet. With lazy parsing, I don't know if it
matters which document you're using to get the use counter, but it seems like
your code might set it to null for lazy parsing after having referenced the
resource from multiple documents.

If the document originally requested and parsed the sheet is disposed and other
instances still holding on to the StyleSheetContents do lazy-parse, the original
owner document is gone, so you're right about needing to update it in the lazy
parsing state, but I think you might need to do what the
clientSingleOwnerDocument does, but just walk the loading/completed clients to
return the first document without checking for a single owner document. That
document will be an arbitrary document, but probably as good as for the initial
parse where the first document parsing it is used (I don't know if
UseCounter<->Document relation matters?)

This is mostly written off-the-top-of-my-head, so the code needs investigation
to confirm.

[Charlie Harrison, 2016-11-30 21:14:23]

Thank you very much for the explanation! I believe you are right about this
code. To summarize my thinking:

With non-lazy parsing, we completely count everything if the initiating document
is a single owner document and we have a single owning node. For the many-client
case, we count nothing.

With lazy parsing without this patch, as soon as we move to a multi-client world
we stop counting, even if the initial lazy parse had a single owner document.

One technique we could use to solve this and keep counts the same would be  to
save a bit "m_wasInitiatedWithSingleOwnerDocumentAndNode" which could be named
"m_shouldTryToUseCount". If the bit is true, we want to do our best to always
count rules (i.e. find *any* document, as you pointed out). If the bit is false,
we can avoid counting all together.

[rune, 2016-12-01 09:18:34]

On 2016/11/30 21:14:23, Charlie Harrison wrote:
> Thank you very much for the explanation! I believe you are right about this
> code. To summarize my thinking:
> 
> With non-lazy parsing, we completely count everything if the initiating
document
> is a single owner document and we have a single owning node. For the
many-client
> case, we count nothing.
> 
> With lazy parsing without this patch, as soon as we move to a multi-client
world
> we stop counting, even if the initial lazy parse had a single owner document.
> 
> One technique we could use to solve this and keep counts the same would be  to
> save a bit "m_wasInitiatedWithSingleOwnerDocumentAndNode" which could be named
> "m_shouldTryToUseCount". If the bit is true, we want to do our best to always
> count rules (i.e. find *any* document, as you pointed out). If the bit is
false,
> we can avoid counting all together.

Do we really need that bit? Aren't all StyleSheetContents first parsed with a
singleOwnerDocument in the non-lazy-parse case?

[Charlie Harrison, 2016-12-01 14:07:21]

On 2016/12/01 09:18:34, rune wrote:
> On 2016/11/30 21:14:23, Charlie Harrison wrote:
> > Thank you very much for the explanation! I believe you are right about this
> > code. To summarize my thinking:
> > 
> > With non-lazy parsing, we completely count everything if the initiating
> document
> > is a single owner document and we have a single owning node. For the
> many-client
> > case, we count nothing.
> > 
> > With lazy parsing without this patch, as soon as we move to a multi-client
> world
> > we stop counting, even if the initial lazy parse had a single owner
document.
> > 
> > One technique we could use to solve this and keep counts the same would be 
to
> > save a bit "m_wasInitiatedWithSingleOwnerDocumentAndNode" which could be
named
> > "m_shouldTryToUseCount". If the bit is true, we want to do our best to
always
> > count rules (i.e. find *any* document, as you pointed out). If the bit is
> false,
> > we can avoid counting all together.
> 
> Do we really need that bit? Aren't all StyleSheetContents first parsed with a
> singleOwnerDocument in the non-lazy-parse case?

I was thinking of something like multiple documents sharing a single
CSSStyleSheetResource before it is fully loaded, but after tracing the code I
think you're right. We should DCHECK this to be sure. Let me amend the patch
with your suggestions.

[rune, 2016-12-01 15:20:45]

On 2016/12/01 14:07:21, Charlie Harrison wrote:
> On 2016/12/01 09:18:34, rune wrote:

> > Do we really need that bit? Aren't all StyleSheetContents first parsed with
a
> > singleOwnerDocument in the non-lazy-parse case?
> 
> I was thinking of something like multiple documents sharing a single
> CSSStyleSheetResource before it is fully loaded, but after tracing the code I
> think you're right. We should DCHECK this to be sure. Let me amend the patch
> with your suggestions.

I'm not sure, but I thought that could only happen for sheets with @imports for
which we don't cache, or something. Needs to be checked.

[Charlie Harrison, 2016-12-02 18:29:51]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 18:30:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-02 18:35:37]

On 2016/12/01 15:20:45, rune wrote:
> On 2016/12/01 14:07:21, Charlie Harrison wrote:
> > On 2016/12/01 09:18:34, rune wrote:
> 
> > > Do we really need that bit? Aren't all StyleSheetContents first parsed
with
> a
> > > singleOwnerDocument in the non-lazy-parse case?
> > 
> > I was thinking of something like multiple documents sharing a single
> > CSSStyleSheetResource before it is fully loaded, but after tracing the code
I
> > think you're right. We should DCHECK this to be sure. Let me amend the patch
> > with your suggestions.
> 
> I'm not sure, but I thought that could only happen for sheets with @imports
for
> which we don't cache, or something. Needs to be checked.

I'm pretty sure even in that case we will have both a singleOwnerDocument and a
singeOwnerNode() (see StyleRuleImport::setCSSStyleSheet).

In any case, I've uploaded a new PS that adds anyOwnerDocument() to
StyleSheetContents, and adds the DCHECKs I mentioned in #16.

[commit-bot: I haz the power, 2016-12-02 20:39:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 20:39:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2016-12-02 22:18:09]

lgtm if nits/issues are fixed or justified.

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:549: :
(*m_completedClients.begin())->ownerDocument();
Nit: is the return rewrite due to the chromium coding style? I think it was more
readable before.

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right):

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:24: if
(document != m_document) {
Couldn't you check if m_document is nullptr before calling anyOwnerDocument()?

Like:

if (!m_document) {
  m_document = m_owningContents->anyOwnerDocument();
  m_context = ...;
}

Or is it a problem if you use the use-counter from a document which is no longer
a client, but before it is garbage collected and the weak reference is not
cleared?

If it's theoretically possible that you enter lazy parsing after all clients
have been cleared, but documents are not garbage collected, the if (document !=
m_document) variant could set m_document from a non-null to a nullptr.

[rune, 2016-12-02 22:19:44]

Just noticed that DCHECKs failed during testing.

[Charlie Harrison, 2016-12-02 22:38:46]

Yeah I'm investigating the DCHECK failures now, thanks for the review.

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:549: :
(*m_completedClients.begin())->ownerDocument();
On 2016/12/02 22:18:08, rune wrote:
> Nit: is the return rewrite due to the chromium coding style? I think it was
more
> readable before.

I can change it back, I was mostly just trying to remove a branch.

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right):

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:24: if
(document != m_document) {
On 2016/12/02 22:18:09, rune wrote:
> Couldn't you check if m_document is nullptr before calling anyOwnerDocument()?
> 
> Like:
> 
> if (!m_document) {
>   m_document = m_owningContents->anyOwnerDocument();
>   m_context = ...;
> }
> 
> Or is it a problem if you use the use-counter from a document which is no
longer
> a client, but before it is garbage collected and the weak reference is not
> cleared?
> 
> If it's theoretically possible that you enter lazy parsing after all clients
> have been cleared, but documents are not garbage collected, the if (document
!=
> m_document) variant could set m_document from a non-null to a nullptr.

Yes I think we can do this. My reason for doing it like this is that it is much
easier to unit test this code. In addition, using a if (!m_document) check is
much faster than pulling the owning document.

[Charlie Harrison, 2016-12-03 06:41:28]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 06:41:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 07:56:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 07:56:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-12-05 15:41:20]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 15:41:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 16:52:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 16:52:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2016-12-05 17:47:45]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 17:48:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 19:26:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 19:26:14]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-12-05 19:30:10]

Since we don't always have a single owner node (and thus we don't always use
count in parseAuthorStyleSheet), I've added a bit to the lazy parsing state if
we should use count.

If we *do* want to use count, we try to as best as possible to find a use
counter from one of the registered clients.

Changing to check for the Weak<Document> caused one of the tests to get
significantly more complicated with forcing oilpan GC, but it's the right thing
to do (simple and fast code over simple tests).

Since the CL is now changing behavior to ensure UseCounting parity, I'll change
the description and title to be more accurate.

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2474483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:549: :
(*m_completedClients.begin())->ownerDocument();
On 2016/12/02 22:38:46, Charlie Harrison wrote:
> On 2016/12/02 22:18:08, rune wrote:
> > Nit: is the return rewrite due to the chromium coding style? I think it was
> more
> > readable before.
> 
> I can change it back, I was mostly just trying to remove a branch.

Done.

[Charlie Harrison, 2016-12-05 19:33:41]

Description was changed from

==========
[LazyParseCSS] Cache the Document as a proxy for UseCounter changes.

For lazy parsing, we need to be careful to update the parsing context's
UseCounter the owning Document swaps out from under us. This patch changes
that logic from using UseCounter::getFrom to using the StyleSheetContents
singleOwnerDocument.

Using UseCounter::getFrom was costing ~4.5% overhead when all the rules are
parsed. Moving to singleOwnerDocument moves this to ~2% overhead.

BUG=642722
==========

to

==========
[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722
==========

[rune, 2016-12-05 22:21:05]

lgtm

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right):

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:50:
UseCounter* useCounter = UseCounter::getFrom(m_document);
If we always retrieve the UseCounter, could we simple just return
CSSParserContext(m_context, UseCounter::getFrom(m_document)) instead? Or would
that be more expensive?

[Charlie Harrison, 2016-12-05 22:27:59]

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right):

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:50:
UseCounter* useCounter = UseCounter::getFrom(m_document);
On 2016/12/05 22:21:05, rune wrote:
> If we always retrieve the UseCounter, could we simple just return
> CSSParserContext(m_context, UseCounter::getFrom(m_document)) instead? Or would
> that be more expensive?

I don't have the trace now, but I remember seeing the context initialization in
profiles before. It isn't a ton, but we have do a KURL copy, a String copy, and
a Referrer copy. These mostly do things like bumping ref counts and copying
simple structs like url::Parsed, but I'd like to avoid it if possible to keep
overhead low and consistent.

[rune, 2016-12-05 22:31:53]

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp (right):

https://codereview.chromium.org/2474483002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSLazyParsingState.cpp:50:
UseCounter* useCounter = UseCounter::getFrom(m_document);
On 2016/12/05 22:27:59, Charlie Harrison wrote:
> On 2016/12/05 22:21:05, rune wrote:
> > If we always retrieve the UseCounter, could we simple just return
> > CSSParserContext(m_context, UseCounter::getFrom(m_document)) instead? Or
would
> > that be more expensive?
> 
> I don't have the trace now, but I remember seeing the context initialization
in
> profiles before. It isn't a ton, but we have do a KURL copy, a String copy,
and
> a Referrer copy. These mostly do things like bumping ref counts and copying
> simple structs like url::Parsed, but I'd like to avoid it if possible to keep
> overhead low and consistent.

Acknowledged.

[Charlie Harrison, 2016-12-05 22:51:46]

Thanks. Sending to CQ

[Charlie Harrison, 2016-12-05 22:51:51]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-12-05 22:52:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 22:58:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480978311891840,
"parent_rev": "c858fb2105d1eeceefa85c2d223130cc9dc9ba00", "commit_rev":
"6e41420081f5c4448f3d66ada24543a5b97f3246"}

[commit-bot: I haz the power, 2016-12-05 22:58:45]

Description was changed from

==========
[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722
==========

to

==========
[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722
==========

[commit-bot: I haz the power, 2016-12-05 22:58:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 23:01:28]

Description was changed from

==========
[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722
==========

to

==========
[LazyParseCSS] Ensure UseCounting has parity with strict parsing

This patch revamps lazy parsing UseCounting to be as safe and accurate
as possible. For sheets that need counting, we always try to find a
valid Document and use its UseCounter when doing any parsing work. We
maintain a WeakMember to the owning Document to ensure our parsing
context stays valid.

BUG=642722

Committed: https://crrev.com/f8090ce1eb0631947f38a75bd27fa8325facf921
Cr-Commit-Position: refs/heads/master@{#436439}
==========

[commit-bot: I haz the power, 2016-12-05 23:01:29]

Patchset 5 (id:??) landed as
https://crrev.com/f8090ce1eb0631947f38a75bd27fa8325facf921
Cr-Commit-Position: refs/heads/master@{#436439}