Oilpan: promptly dispose style resolvers upon clearing.

Oilpan: promptly dispose style resolvers upon clearing.

Try to balance the tables somewhat on storage allocation reuse for
Oilpan wrt (Scoped)StyleResolver:

 - when the StyleEngine clears out its resolver, use that end-of-life
   signal to (promptly) release all its collections. This might bring
   about collection backing storage reuse.
 - similarly for TreeScope and its ScopedStyleResolver; explicitly
   dispose() the resolver object being cleared.

R=
BUG=

[haraken, 2015-10-08 06:15:45]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-10-08 06:15:46]

Given that this code is for oilpan-only, let's add ENABLE(OILPAN).

One question: Does blink_style.top_25 create so many StyleResolvers? The fact
that StyleResolver::dispose improves performance implies that blink_style.top_25
disposes a lot of StyleResolvers, but that is a bit surprising to me.

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

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:126: void RuleSet::dispose()

Shall we add ENABLE(OILPAN)?

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h (right):

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h:53: void
dispose();

Ditto.

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleResolver.h:91: void dispose();

Ditto.

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/1382353002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:430: #if ENABLE(OILPAN)

Add a comment about why we need to call dispose explicitly.

[sof, 2015-10-08 06:24:40]

On 2015/10/08 06:15:46, haraken wrote:
> Given that this code is for oilpan-only, let's add ENABLE(OILPAN).
> 
> One question: Does blink_style.top_25 create so many StyleResolvers? The fact
> that StyleResolver::dispose improves performance implies that
blink_style.top_25
> disposes a lot of StyleResolvers, but that is a bit surprising to me.
> 

https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...
has the code it runs to create the style resolvers & invalidations

[haraken, 2015-10-08 07:23:45]

On 2015/10/08 06:24:40, sof wrote:
> On 2015/10/08 06:15:46, haraken wrote:
> > Given that this code is for oilpan-only, let's add ENABLE(OILPAN).
> > 
> > One question: Does blink_style.top_25 create so many StyleResolvers? The
fact
> > that StyleResolver::dispose improves performance implies that
> blink_style.top_25
> > disposes a lot of StyleResolvers, but that is a bit surprising to me.
> > 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...
> has the code it runs to create the style resolvers & invalidations

Thanks, understood.

Now I begin to think that it would be better to not land this CL. Since it would
be rare that a ton of StyleResolvers are created & disposed in a short period of
time, the CL is just gaming micro-benchmarks. Instead of doing that, we can just
justify the regression when sending a ship announcement of oilpan.

What do you think?

(It is a great progress that you identified that the regression is not a real
one.)

[sof, 2015-10-08 07:34:27]

On 2015/10/08 07:23:45, haraken wrote:
> On 2015/10/08 06:24:40, sof wrote:
> > On 2015/10/08 06:15:46, haraken wrote:
> > > Given that this code is for oilpan-only, let's add ENABLE(OILPAN).
> > > 
> > > One question: Does blink_style.top_25 create so many StyleResolvers? The
> fact
> > > that StyleResolver::dispose improves performance implies that
> > blink_style.top_25
> > > disposes a lot of StyleResolvers, but that is a bit surprising to me.
> > > 
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...
> > has the code it runs to create the style resolvers & invalidations
> 
> Thanks, understood.
> 
> Now I begin to think that it would be better to not land this CL. Since it
would
> be rare that a ton of StyleResolvers are created & disposed in a short period
of
> time, the CL is just gaming micro-benchmarks. Instead of doing that, we can
just
> justify the regression when sending a ship announcement of oilpan.
> 
> What do you think?
> 
> (It is a great progress that you identified that the regression is not a real
> one.)

rune@ mentioned that components containing their own styles will trigger
reconstruction of style information & clearResolver() being called. I don't
know, but one would think this will happen repeatedly upon initial handling of a
document with several such components.

Regardless, I'm happy to take a wait&see on the need for this optimization.

[haraken, 2015-10-08 07:39:06]

On 2015/10/08 07:34:27, sof wrote:
> On 2015/10/08 07:23:45, haraken wrote:
> > On 2015/10/08 06:24:40, sof wrote:
> > > On 2015/10/08 06:15:46, haraken wrote:
> > > > Given that this code is for oilpan-only, let's add ENABLE(OILPAN).
> > > > 
> > > > One question: Does blink_style.top_25 create so many StyleResolvers? The
> > fact
> > > > that StyleResolver::dispose improves performance implies that
> > > blink_style.top_25
> > > > disposes a lot of StyleResolvers, but that is a bit surprising to me.
> > > > 
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...
> > > has the code it runs to create the style resolvers & invalidations
> > 
> > Thanks, understood.
> > 
> > Now I begin to think that it would be better to not land this CL. Since it
> would
> > be rare that a ton of StyleResolvers are created & disposed in a short
period
> of
> > time, the CL is just gaming micro-benchmarks. Instead of doing that, we can
> just
> > justify the regression when sending a ship announcement of oilpan.
> > 
> > What do you think?
> > 
> > (It is a great progress that you identified that the regression is not a
real
> > one.)
> 
> rune@ mentioned that components containing their own styles will trigger
> reconstruction of style information & clearResolver() being called. I don't
> know, but one would think this will happen repeatedly upon initial handling of
a
> document with several such components.
> 
> Regardless, I'm happy to take a wait&see on the need for this optimization.

Makes sense!

Either way, it's now clear that this is no longer a blocker for starting the
final performance measurement. Let's start the measurement. We can land this CL
when necessary (before we actually enable oilpan by default on trunk).

[sof, 2015-10-08 07:42:37]

On 2015/10/08 07:39:06, haraken wrote:
> On 2015/10/08 07:34:27, sof wrote:
> > On 2015/10/08 07:23:45, haraken wrote:
> > > On 2015/10/08 06:24:40, sof wrote:
> > > > On 2015/10/08 06:15:46, haraken wrote:
> > > > > Given that this code is for oilpan-only, let's add ENABLE(OILPAN).
> > > > > 
> > > > > One question: Does blink_style.top_25 create so many StyleResolvers?
The
> > > fact
> > > > > that StyleResolver::dispose improves performance implies that
> > > > blink_style.top_25
> > > > > disposes a lot of StyleResolvers, but that is a bit surprising to me.
> > > > > 
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...
> > > > has the code it runs to create the style resolvers & invalidations
> > > 
> > > Thanks, understood.
> > > 
> > > Now I begin to think that it would be better to not land this CL. Since it
> > would
> > > be rare that a ton of StyleResolvers are created & disposed in a short
> period
> > of
> > > time, the CL is just gaming micro-benchmarks. Instead of doing that, we
can
> > just
> > > justify the regression when sending a ship announcement of oilpan.
> > > 
> > > What do you think?
> > > 
> > > (It is a great progress that you identified that the regression is not a
> real
> > > one.)
> > 
> > rune@ mentioned that components containing their own styles will trigger
> > reconstruction of style information & clearResolver() being called. I don't
> > know, but one would think this will happen repeatedly upon initial handling
of
> a
> > document with several such components.
> > 
> > Regardless, I'm happy to take a wait&see on the need for this optimization.
> 
> Makes sense!
> 
> Either way, it's now clear that this is no longer a blocker for starting the
> final performance measurement. Let's start the measurement. We can land this
CL
> when necessary (before we actually enable oilpan by default on trunk).

Sounds like a good plan; if there is a mild regression against
blink_style.top25, we can link it against this CL as a possible mitigator, if
wished for.

[sof, 2015-10-08 08:50:01]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-10-08 08:50:02]

Updated with ENABLE(OILPAN) protections + added explanatory comments.