[Invalidation Tracking] Trace ScheduleStyleInvalidation

[Invalidation Tracking] Trace ScheduleStyleInvalidation

This patch adds a set of trace events to track reason when RuleFeatureSet
schedules StyleInvalidator invocations.
These schedules are later consumed by StyleInvalidator to setNeedsStyleRecalc
for nodes invalidated by the selector changes.

These trace events are postprocessed in devtools and connected to StyleInvalidator
invalidate events by joining their DescendantInvalidationSet ids.

BUG=410701
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184231

[kouhei (in TOK), 2014-10-20 06:50:58]

kouhei@chromium.org changed reviewers:
+ caseq@chromium.org, haraken@chromium.org, pdr@chromium.org

[kouhei (in TOK), 2014-10-20 06:50:58]

pdr: Would you take a high level look?

This regresses CSS benchmarks by 1.5%, so I'm currently working on an
optimization similar to StyleInvalidator.

[kouhei (in TOK), 2014-10-20 07:02:45]

Uploaded updated patch w/o regression. PTAL.

[rune, 2014-10-20 07:24:02]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2014-10-20 07:24:03]

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:203: s_tracingEnabled =
TRACE_EVENT_API_GET_CATEGORY_ENABLED(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"));
This looks like a strange place to initialize that variable. RuleFeatures are
typically not created after you loaded your document. If I want to do tracing
with invalidationTracking enabled, I would typically do that after I loaded the
document to check the style invalidation effect of some user interaction?

[kouhei (in TOK), 2014-10-20 09:06:20]

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:203: s_tracingEnabled =
TRACE_EVENT_API_GET_CATEGORY_ENABLED(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"));
On 2014/10/20 07:24:03, rune wrote:
> This looks like a strange place to initialize that variable. RuleFeatures are
> typically not created after you loaded your document. If I want to do tracing
> with invalidationTracking enabled, I would typically do that after I loaded
the
> document to check the style invalidation effect of some user interaction?

Thanks for your comment.

This is a bit confusing (i'll add a comment later), but this is only caching the
pointer to the flag, so the flag itself can be changed afterwards.
The location of the flag doesn't change since tracing backend is initialized, so
ideally this can be done only once per renderer process.

[rune, 2014-10-20 09:18:18]

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:203: s_tracingEnabled =
TRACE_EVENT_API_GET_CATEGORY_ENABLED(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"));
On 2014/10/20 09:06:20, kouhei wrote:
> On 2014/10/20 07:24:03, rune wrote:
> > This looks like a strange place to initialize that variable. RuleFeatures
are
> > typically not created after you loaded your document. If I want to do
tracing
> > with invalidationTracking enabled, I would typically do that after I loaded
> the
> > document to check the style invalidation effect of some user interaction?
> 
> Thanks for your comment.
> 
> This is a bit confusing (i'll add a comment later), but this is only caching
the
> pointer to the flag, so the flag itself can be changed afterwards.
> The location of the flag doesn't change since tracing backend is initialized,
so
> ideally this can be done only once per renderer process.

Yes, I see that now. Still, I think it would be better if this was exported as a
fast api from the tracing code instead if possible.

[kouhei (in TOK), 2014-10-20 09:24:31]

On 2014/10/20 09:18:18, rune wrote:
>
https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
> File Source/core/css/RuleFeature.cpp (right):
> 
>
https://codereview.chromium.org/664033002/diff/20001/Source/core/css/RuleFeat...
> Source/core/css/RuleFeature.cpp:203: s_tracingEnabled =
>
TRACE_EVENT_API_GET_CATEGORY_ENABLED(TRACE_DISABLED_BY_DEFAULT("devtools.timeline.invalidationTracking"));
> On 2014/10/20 09:06:20, kouhei wrote:
> > On 2014/10/20 07:24:03, rune wrote:
> > > This looks like a strange place to initialize that variable. RuleFeatures
> are
> > > typically not created after you loaded your document. If I want to do
> tracing
> > > with invalidationTracking enabled, I would typically do that after I
loaded
> > the
> > > document to check the style invalidation effect of some user interaction?
> > 
> > Thanks for your comment.
> > 
> > This is a bit confusing (i'll add a comment later), but this is only caching
> the
> > pointer to the flag, so the flag itself can be changed afterwards.
> > The location of the flag doesn't change since tracing backend is
initialized,
> so
> > ideally this can be done only once per renderer process.
> 
> Yes, I see that now. Still, I think it would be better if this was exported as
a
> fast api from the tracing code instead if possible.

Would you tell me your idea (fast api) with more detail?
I totally agree that the current code is a hack, and this patch is not super
urgent, so I'm happy to rewrite this in a less hacky way.
Only justification I have is that this pattern is already used in Chromium side,
but we can change those too if we come up with a better idea.

[rune, 2014-10-20 09:31:04]

On 2014/10/20 09:24:31, kouhei wrote:
> On 2014/10/20 09:18:18, rune wrote:

> > Yes, I see that now. Still, I think it would be better if this was exported
as
> a
> > fast api from the tracing code instead if possible.
> 
> Would you tell me your idea (fast api) with more detail?
> I totally agree that the current code is a hack, and this patch is not super
> urgent, so I'm happy to rewrite this in a less hacky way.
> Only justification I have is that this pattern is already used in Chromium
side,
> but we can change those too if we come up with a better idea.

Just to move that global pointer to the tracing code, initialize it once, and
wrap the access to it in a method (possibly inlined) exported from the tracing
code.

[kouhei (in TOK), 2014-10-20 10:40:05]

On 2014/10/20 09:31:04, rune wrote:
> On 2014/10/20 09:24:31, kouhei wrote:
> > On 2014/10/20 09:18:18, rune wrote:
> 
> > > Yes, I see that now. Still, I think it would be better if this was
exported
> as
> > a
> > > fast api from the tracing code instead if possible.
> > 
> > Would you tell me your idea (fast api) with more detail?
> > I totally agree that the current code is a hack, and this patch is not super
> > urgent, so I'm happy to rewrite this in a less hacky way.
> > Only justification I have is that this pattern is already used in Chromium
> side,
> > but we can change those too if we come up with a better idea.
> 
> Just to move that global pointer to the tracing code, initialize it once, and
> wrap the access to it in a method (possibly inlined) exported from the tracing
> code.

Thanks for the clarification. I'll try that and investigate more tomorrow.

Last time I tried the cache pointer as an extern global pointer on
InspectorTraceEvents,
it was too slow from cost of exported symbol lookup,
but traceevents on this CL seems to be less hot than TraceInvalidator hooks.
I'm running the benchmarks again with different level of hacks.

[kouhei (in TOK), 2014-10-22 01:40:49]

PTAL

> Thanks for the clarification. I'll try that and investigate more tomorrow.
I run the benchmark several times, but the results seem too flaky to judge 1%ish
regression.
I'm a perf sheriff on Fri and Mon, so I'd like to land this CL without
optimization and see what happens.

[rune, 2014-10-22 07:11:51]

core/css lgtm

[kouhei (in TOK), 2014-10-22 07:13:23]

caseq, pdr: Would you approve core/inspector?

> core/css lgtm
Thanks!

[pdr., 2014-10-22 17:14:35]

LGTM

https://codereview.chromium.org/664033002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/664033002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:171: /* blocked on
https://codereview.chromium.org/654013003/
This should have landed now

[kouhei (in TOK), 2014-10-23 02:09:50]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-10-23 02:11:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/664033002/60001

[kouhei (in TOK), 2014-10-23 03:23:32]

https://codereview.chromium.org/664033002/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/664033002/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:171: /* blocked on
https://codereview.chromium.org/654013003/
On 2014/10/22 17:14:35, pdr wrote:
> This should have landed now

Done.

[commit-bot: I haz the power, 2014-10-23 03:53:25]

Committed patchset #4 (id:60001) as 184231