Use weak members to cache StyleSheetContents.

Use weak members to cache StyleSheetContents.

We used the client count to detect if we could remove a
StyleSheetContents from the StyleEngine cache or not. The problem is
that the client references are removed when the element is removed from
the DOM, but the StyleSheetContents is still referenced from the
CSSStyleSheet which is accessible from CSSOM. That caused bugs with
StyleSheetContents being marked as mutable without removing it from the
cache causing assertions, and mutating the sheet without copy-on-write
because we thought we only had a single client for the contents.

Instead use weak members in the cache and let garbage collection delete
the StyleSheetContents when no longer referenced. Also, add a flag to
StyleSheetContents to say that it is referenced by multiple sheets when
we use and already cached object instead of incorrectly relying on
client count.

R=timloh@chromium.org,haraken@chromium.org
BUG=633210, 628488
Committed: https://crrev.com/473ac0866d46d55bf57293ce3bb27870f672dc40
Cr-Commit-Position: refs/heads/master@{#409495}

[rune, 2016-08-02 11:58:53]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 11:59:06]

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-08-02 12:28:44]

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

[commit-bot: I haz the power, 2016-08-02 12:28:45]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[rune, 2016-08-02 12:39:34]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 12:39:52]

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-08-02 13:37:55]

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

[commit-bot: I haz the power, 2016-08-02 13:37:55]

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

[rune, 2016-08-02 17:54:46]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 17:55:23]

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-08-02 19:27:30]

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

[commit-bot: I haz the power, 2016-08-02 19:27:31]

Dry run: This issue passed the CQ dry run.

[rune, 2016-08-02 21:20:33]

ptal

@haraken, please take a look at the WeakMember usage in the StyleEngine caches
and see if it makes sense. Some testing I've done indicates that HeapHashMap
entries are removed when WeakMembers are collected, both when used as keys and
values.

[haraken, 2016-08-03 01:01:18]

The WeakMember usage looks good, but isn't there any case where you need to
explicitly remove the sheet from the cache? i.e., is it sufficient to remove the
sheet from the cache only when it becomes unreachable?

https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (left):

https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:103:
removeSheetFromCache(document);

For example, don't you need to explicitly remove the sheet from the cache here?

[rune, 2016-08-03 09:20:05]

https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (left):

https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:103:
removeSheetFromCache(document);
On 2016/08/03 01:01:18, haraken wrote:
> 
> For example, don't you need to explicitly remove the sheet from the cache
here?

I think that removeSheetFromCache here never removed anything because ending up
here for a stylesheet which is already in the cache should be guarded by a
RuleMutationScope which calls willMutateRules(). willMutateRules() used to
remove the StyleSheetContents from the cache. I have removed the explicit remove
from there as well. What happens in willMutateRules() is that the contents will
be setMutable() if there's only one sheet referring to it, and otherwise cloned
before modified. If the in-cache contents is set mutable, it will not be
cacheable and the cache-entry will be replaced if we try to create a sheet with
the same text again.

[haraken, 2016-08-03 09:25:01]

On 2016/08/03 09:20:05, rune wrote:
>
https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (left):
> 
>
https://codereview.chromium.org/2205843003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/StyleSheetContents.cpp:103:
> removeSheetFromCache(document);
> On 2016/08/03 01:01:18, haraken wrote:
> > 
> > For example, don't you need to explicitly remove the sheet from the cache
> here?
> 
> I think that removeSheetFromCache here never removed anything because ending
up
> here for a stylesheet which is already in the cache should be guarded by a
> RuleMutationScope which calls willMutateRules(). willMutateRules() used to
> remove the StyleSheetContents from the cache. I have removed the explicit
remove
> from there as well. What happens in willMutateRules() is that the contents
will
> be setMutable() if there's only one sheet referring to it, and otherwise
cloned
> before modified. If the in-cache contents is set mutable, it will not be
> cacheable and the cache-entry will be replaced if we try to create a sheet
with
> the same text again.

Thanks, makes sense. LGTM.

[rune, 2016-08-03 09:38:34]

The CQ bit was checked by rune@opera.com

[rune, 2016-08-03 09:38:34]

The CQ bit was unchecked by rune@opera.com

[rune, 2016-08-03 09:38:42]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-08-03 09:38:57]

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-08-03 09:45:40]

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

[commit-bot: I haz the power, 2016-08-03 09:45:42]

Internal error: failed to checkout. Please try again.

[rune, 2016-08-03 12:14:14]

The CQ bit was checked by rune@opera.com

[rune, 2016-08-03 12:14:14]

The CQ bit was unchecked by rune@opera.com

[rune, 2016-08-03 12:14:21]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-08-03 12:14:37]

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-08-03 12:18:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-03 12:19:50]

Description was changed from

==========
Use weak members to cache StyleSheetContents.

We used the client count to detect if we could remove a
StyleSheetContents from the StyleEngine cache or not. The problem is
that the client references are removed when the element is removed from
the DOM, but the StyleSheetContents is still referenced from the
CSSStyleSheet which is accessible from CSSOM. That caused bugs with
StyleSheetContents being marked as mutable without removing it from the
cache causing assertions, and mutating the sheet without copy-on-write
because we thought we only had a single client for the contents.

Instead use weak members in the cache and let garbage collection delete
the StyleSheetContents when no longer referenced. Also, add a flag to
StyleSheetContents to say that it is referenced by multiple sheets when
we use and already cached object instead of incorrectly relying on
client count.

R=timloh@chromium.org,haraken@chromium.org
BUG=633210,628488
==========

to

==========
Use weak members to cache StyleSheetContents.

We used the client count to detect if we could remove a
StyleSheetContents from the StyleEngine cache or not. The problem is
that the client references are removed when the element is removed from
the DOM, but the StyleSheetContents is still referenced from the
CSSStyleSheet which is accessible from CSSOM. That caused bugs with
StyleSheetContents being marked as mutable without removing it from the
cache causing assertions, and mutating the sheet without copy-on-write
because we thought we only had a single client for the contents.

Instead use weak members in the cache and let garbage collection delete
the StyleSheetContents when no longer referenced. Also, add a flag to
StyleSheetContents to say that it is referenced by multiple sheets when
we use and already cached object instead of incorrectly relying on
client count.

R=timloh@chromium.org,haraken@chromium.org
BUG=633210,628488

Committed: https://crrev.com/473ac0866d46d55bf57293ce3bb27870f672dc40
Cr-Commit-Position: refs/heads/master@{#409495}
==========

[commit-bot: I haz the power, 2016-08-03 12:19:51]

Patchset 3 (id:??) landed as
https://crrev.com/473ac0866d46d55bf57293ce3bb27870f672dc40
Cr-Commit-Position: refs/heads/master@{#409495}

[kouhei (in TOK), 2016-08-07 12:15:26]

On 2016/08/03 12:19:51, commit-bot: I haz the power wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/473ac0866d46d55bf57293ce3bb27870f672dc40
> Cr-Commit-Position: refs/heads/master@{#409495}

This CL hugely improved the StyleSheetInsert* perf benchmarks. Congrats!
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fu...

[rune, 2016-08-07 17:28:35]

On 2016/08/07 12:15:26, kouhei wrote:
> On 2016/08/03 12:19:51, commit-bot: I haz the power wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/473ac0866d46d55bf57293ce3bb27870f672dc40
> > Cr-Commit-Position: refs/heads/master@{#409495}
> 
> This CL hugely improved the StyleSheetInsert* perf benchmarks. Congrats!
>
https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fu...

That's not intentional, but looking at the test I know why it happens. It's the
same stylesheet text being repeatedly inserted and removed. Before my change,
the cache entry would be removed immediately when removed and the stylesheet
would have to be reparsed every time. With this CL, the weak reference is not
collected until the next time the sheet is created, and it can utilize the
cached entry.