Issue 654013003: Implement invalidation tracking in devtools

Issue 654013003: Implement invalidation tracking in devtools (Closed)

Created:
6 years, 2 months ago by pdr.

Modified:
6 years, 2 months ago

Reviewers:
caseq, apavlov, aandrey, kouhei (in TOK), yurys

CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, eustas+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, paulirish+reviews_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, vsevik+blink_chromium.org, yurys+blink_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Implement invalidation tracking in devtools This patch adds the first bits of the invalidation tracking experiment. Our full design doc is at: https://docs.google.com/document/d/1BXT3_gD--YdbIYMlTzNGsyUUMBCZJ7V5qGJhMA-pGrs/edit This patch implements the invalidation tracking logic for paint events only. Layout, style recalc, and compositing updates will be instrumented in a followup. To keep this patch simple, only the basic invalidation information has been added. Additional style invalidation information (https://src.chromium.org/viewvc/blink?view=rev&revision=183643) will be plumbed through in a followup. Reviewers are encouraged to play with this patch. Steps for testing: 1) Go to about://flags and enable "Enable Developer Tools experiments." 2) Restart Chrome. 3) Start Chrome, open devtools, and click the sprocket. Select "Experiments" and enable "Timeline with full invalidation tracking". 4) Restart devtools (or open a new tab). 5) Open your favorite slow page (or try http://pr.gg/jscsslayout.html), open devtools, start the timeline with the "Causes" checkbox checked. 6) Click the Paint events in the timeline, notice invalidation data! A screenshot preview is available at: http://pr.gg/invalidations7.png BUG=410701 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184041

Patch Set 1 #

Total comments: 45

Patch Set 2 : Update per reviewer comments #

Patch Set 3 : Minor cleanup of case statement #

Total comments: 16

Patch Set 4 : Address reviewer comments, update how stacks are handled #

Total comments: 8

Patch Set 5 : Update per reviewer comments #

Patch Set 6 : Fix test to force layout in the correct frame #

Patch Set 7 : Update test for landing #

Total comments: 1

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+594 lines, -10 lines)			Patch
M	LayoutTests/inspector/tracing/timeline-enum-stability-expected.txt	View	1	2 chunks	+4 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html	View	1 2 3 4 5 6	1 chunk	+101 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations-expected.txt	View		1 chunk	+8 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-layout-invalidations.html	View	1 2 3 4 5	1 chunk	+94 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-layout-invalidations-expected.txt	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-style-recalc-invalidations.html	View	1 2 3	1 chunk	+87 lines, -0 lines	0 comments	Download
A	LayoutTests/inspector/tracing/timeline-style-recalc-invalidations-expected.txt	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
M	Source/core/inspector/InspectorTraceEvents.cpp	View	1 2 3	2 chunks	+4 lines, -2 lines	1 comment	Download
M	Source/devtools/front_end/main/Main.js	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/devtools/front_end/timeline/TimelineUIUtils.js	View	1 2 3 4	1 chunk	+9 lines, -1 line	0 comments	Download
M	Source/devtools/front_end/timeline/TracingTimelineModel.js	View	1 2 3 4	6 chunks	+119 lines, -1 line	0 comments	Download
M	Source/devtools/front_end/timeline/TracingTimelineUIUtils.js	View	1 2 3 4	5 chunks	+143 lines, -6 lines	0 comments	Download
M	Source/platform/TracedValue.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/platform/TracedValue.cpp	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 29 (8 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

pdr.

Caseq, Kouhei, would you be up for an initial review of this patch?

6 years, 2 months ago (2014-10-15 04:59:28 UTC) #2

kouhei (in TOK)

Exciting! https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineUIUtils.js File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right): https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineUIUtils.js#newcode731 Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:731: var styleInvalidations = []; We might want to ...

6 years, 2 months ago (2014-10-15 05:34:00 UTC) #3

caseq

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineModel.js File Source/devtools/front_end/timeline/TracingTimelineModel.js (right): https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineModel.js#newcode148 Source/devtools/front_end/timeline/TracingTimelineModel.js:148: disabledByDefault("devtools.timeline.invalidationTracking")]); make the latter conditional on Runtime.experiments.isEnabled("timelineInvalidationTracking")? https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineModel.js#newcode605 Source/devtools/front_end/timeline/TracingTimelineModel.js:605: ...

6 years, 2 months ago (2014-10-15 16:24:54 UTC) #4

pdr.

Thanks for the reviews! PTAL https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineModel.js File Source/devtools/front_end/timeline/TracingTimelineModel.js (right): https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/timeline/TracingTimelineModel.js#newcode148 Source/devtools/front_end/timeline/TracingTimelineModel.js:148: disabledByDefault("devtools.timeline.invalidationTracking")]); On 2014/10/15 at ...

6 years, 2 months ago (2014-10-16 07:46:48 UTC) #5

Thanks for the reviews! PTAL

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TracingTimelineModel.js (right):

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineModel.js:148:
disabledByDefault("devtools.timeline.invalidationTracking")]);
On 2014/10/15 at 16:24:52, caseq wrote:
> make the latter conditional on
Runtime.experiments.isEnabled("timelineInvalidationTracking")?

Great catch. This was a bad merge on my part :(

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1159:
WebInspector.InvalidationTracker = function()
On 2014/10/15 at 16:24:53, caseq wrote:
> I wonder if we should make it a part of _processEvent logic -- we already have
a ton of similar tracking logic there and you may eventually even benefit from
sharing the state with other record types.

Would you mind if we punt on this for now? I'm not quite sure how adding
invalidations to layout/stylerecalc/updatelayers will work and having this off
to the side may make that easier (or harder, and I'll roll it all up into
_processEvent).

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1182: switch
(invalidationToUpdate.type) {
On 2014/10/15 at 16:24:52, caseq wrote:
> Can we defer making it a "switch" till we have to and keep it as "if" yet
(BTW, switch turns out to be much slower than if last time we checked jsperf).

Looks like 'switch' and 'if/else' are the same these days:
http://jsperf.com/switch-vs-if-conditions

You're right that this would be cleaner with an 'if' though. Fixed, and cleaned
up the long lines with a 'recordTypes' variable.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1200: // If a paint
doesn't have a corresponding graphics layer id, it paints into it's parent so
On 2014/10/15 at 16:24:53, caseq wrote:
> s/it's/its/

Stupid English!

Fixed.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1235:
this._invalidationEvents = [];
On 2014/10/15 at 16:24:52, caseq wrote:
> Looks like if we add new state variable, we'll be adding it both here and in
the constructor, so I would extract this into a method.

Great point. Factored out into _initializePerFrameState.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:497: if (target &&
event.invalidationTrackingEvents)
On 2014/10/15 at 16:24:54, caseq wrote:
> Please drop test for target (the one above is likely a legacy, targets
shouldn't be nullable now, I think).

Fixed, and removed the two above.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:731: var
styleInvalidations = [];
On 2014/10/15 at 16:24:53, caseq wrote:
> On 2014/10/15 05:34:00, kouhei wrote:
> > We might want to use a map of arrays here, as we expect to have more
> > invalidation event types in future.
> 
> +1

Excellent point. Refactored!

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:750: if
(styleInvalidations.length)
On 2014/10/15 at 16:24:54, caseq wrote:
> style: here and below, please use {} for multi-line statements under if.

Refactored this to be much cleaner. I'm not sure who wrote this gross code.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:798: var callstack
= invalidation.callstack ? JSON.parse(invalidation.callstack) : [];
On 2014/10/15 at 16:24:53, caseq wrote:
> I think we need to capture callstacks the usual way we do (i.e. an adjacent
trace event that produces a structured call stack using 
InspectorCallStackEvent)

I wasn't able to find the benefit of this, but I may misunderstand. Could you
explain a bit more?

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:808: var urlElement
= linkifier.linkifyScriptLocation(target, stackFrame.scriptId, stackFrame.url,
stackFrame.lineNumber - 1, stackFrame.columnNumber - 1);
On 2014/10/15 at 05:34:00, kouhei wrote:
> This part looks similar to
WebInspector.TimelineDetailsContentHelper.appendStackTrace. dedupe?

Factored this out into the content helper itself.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:829: });
On 2014/10/15 at 05:34:00, kouhei wrote:
> This looks a bit complicated. If Object.keys(backendNodeIdSet).length is small
enough, can we just keep an array of int and then uniq it?

This is the simplest way I know how to create a set in javascript. Can you
expand on this a bit more concretely? Unfortunately, javascript arrays don't
have a nice unique function.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:833:
backendNodeIds.push(parseInt(backendNodeId));
On 2014/10/15 at 05:34:00, kouhei wrote:
> in other words, can we only use backendNodeIds?

It's the opposite--we can only use frontendNodeIds in the inspector UI, but we
only have backend node ids from tracing. This function is just handling the
asynchronous calls to do the mapping from backend to frontend. We could even
delete the nodeId after updating the frontend node id.

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:836:
backendNodeIds, barrier.createCallback(updateInvalidationNodeIds));
On 2014/10/15 at 16:24:53, caseq wrote:
> You're calling barrier.createCallback() exactly once, so perhaps just pass a
callback, not barrier?

I think we need to use the barrier as it increments the barrier's pending
callback count and prevents callWhenDone from being called up in the caller.

caseq

On 2014/10/15 at 16:24:53, caseq wrote: >> I wonder if we should make it a ...

6 years, 2 months ago (2014-10-16 17:08:01 UTC) #6

On 2014/10/15 at 16:24:53, caseq wrote:
>> I wonder if we should make it a part of _processEvent logic --
>> we already have a ton of similar tracking logic there and you may 
>> eventually even benefit from
>> sharing the state with other record types.

> Would you mind if we punt on this for now? I'm not quite sure how 
> adding invalidations to layout/stylerecalc/updatelayers will work and > having
this off to the side may make that easier (or harder, and I'll 
> roll it all up into _processEvent).

No, I'm fine with them in a separate class as well. In fact, we may actually
want to move the rest into a sort of "processor" class to better control the
life-time of processing-specific state, so the merge perhaps may happen in the
opposite direction. 

Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:798: var callstack
= invalidation.callstack ? JSON.parse(invalidation.callstack) : [];
On 2014/10/15 at 16:24:53, caseq wrote:
>> I think we need to capture callstacks the usual way we do (i.e. an adjacent
trace event that produces a structured call stack using 
InspectorCallStackEvent)

> I wasn't able to find the benefit of this, but I may misunderstand. 
> Could you explain a bit more?

In my view, it falls in two parts: one I assume is quite evident, we shouldn't
do excessive JSON.stringify()/JSON.parse() on something that is already a JSON
-- there seems to be no reason for that, it complicates things and does not help
performance; another is more a knee-jerk reflex to keep things consistent with
existing code, though I'm not sure why we originally chose to send stacks as
separate events (easy to control stack collection via a separate tracing
category, perhaps?). So I don't really insist on the latter, but let's hear from
yurys@ as he picked the existing way to send stacks in a separate event over
making them a data field.

Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:829: });
On 2014/10/15 at 05:34:00, kouhei wrote:
>> This looks a bit complicated. If  
>> Object.keys(backendNodeIdSet).length is small
>> enough, can we just keep an array of int and then uniq it?

> This is the simplest way I know how to create a set in javascript.
> Can you expand on this a bit more concretely? Unfortunately, 
> javascript arrays don't have a nice unique function.

I don't have a position on this (perhaps we should eventually unify this code
with a quite similar one in LayerTreeModel), but FWIW, we avoid parseInt over
object keys elsewhere by doing stuff like

if (!seen[id]) {
    seen[id] = true;
    idsToRequest.push(id)
}

(That happened after code review, my implementation was similar to pdr's,
though).

> It's the opposite--we can only use frontendNodeIds in the inspector > UI, but
we only have backend node ids from tracing. This function is just handling the
> asynchronous calls to do the mapping from backend to frontend. We > could even
> delete the nodeId after updating the frontend node id.

Another $0.02 -- if a node is removed from the DOM and then re-inserted at some
point after we capture the backend  id, the backend id will remain the same,
while the front-end id for the node will be different (so re-resolving backend
node ids is actually good).

https://codereview.chromium.org/654013003/diff/1/Source/devtools/front_end/ti...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:836:
backendNodeIds, barrier.createCallback(updateInvalidationNodeIds));
On 2014/10/15 at 16:24:53, caseq wrote:
>> You're calling barrier.createCallback() exactly once, so perhaps just pass a
callback, not barrier?

> I think we need to use the barrier as it increments the barrier's pending
> callback count and prevents callWhenDone from being called up in the caller.

Sure, we do need to use the barrier, it's just that it doesn't have to be in the
interface here, what I  propose is just call barrier.createCallback() at the
call site and pass the resulting callback into this function, so it matches the
general look of async functions around.

https://codereview.chromium.org/654013003/diff/40001/LayoutTests/inspector/tr...
File
LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html
(right):

https://codereview.chromium.org/654013003/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html:11:
var element = document.getElementsByTagName('p')[0];
nit: assign an id and use getElementById(), it's easier to understand and more
stable.

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelineUIUtils.js (right):

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelineUIUtils.js:569: * @return {!Element}
parentElement
s/@return/@param?

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineModel.js (right):

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1164:
this._invalidationEvents.forEach(function(invalidationToUpdate) {
Here and below: we normally avoid non-trivialfunction literals in devtools
front-end, please extract into a named function and pass its name to forEach()
(closures per se are totally fine!).

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1167: if
(invalidationToUpdate.type == recordTypes.StyleRecalcInvalidationTracking
===

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1168: ||
invalidationToUpdate.type == recordTypes.LayoutInvalidationTracking) {
===

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:1196:
this._invalidationEvents.forEach(function(invalidation) {
ditto -- please make it a named function.

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:783: var reason =
invalidation.reason + (invalidation.reason.endsWith('.') ? '' : '.');
s/'/"/g, though it would be perhaps nice to just make whatever we send from
back-end consistent.

https://codereview.chromium.org/654013003/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:821:
console.error('Did not resolve the correct number of frontend node ids.');
s/'/"/g

pdr.

https://codereview.chromium.org/654013003/diff/40001/LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html File LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html (right): https://codereview.chromium.org/654013003/diff/40001/LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html#newcode11 LayoutTests/inspector/tracing/timeline-layout-deleted-node-invalidations.html:11: var element = document.getElementsByTagName('p')[0]; On 2014/10/16 at 17:08:00, caseq ...

6 years, 2 months ago (2014-10-17 05:07:04 UTC) #7

pdr.

Thanks for the reviews! PTAL On 2014/10/16 at 17:08:01, caseq wrote: > In my view, ...

6 years, 2 months ago (2014-10-17 05:16:23 UTC) #10

yurys

On 2014/10/17 05:16:23, pdr wrote: > Thanks for the reviews! PTAL > > On 2014/10/16 ...

6 years, 2 months ago (2014-10-17 05:48:11 UTC) #11

On 2014/10/17 05:16:23, pdr wrote:
> Thanks for the reviews! PTAL
> 
> On 2014/10/16 at 17:08:01, caseq wrote:
> > In my view, it falls in two parts: one I assume is quite evident, we
shouldn't
> do excessive JSON.stringify()/JSON.parse() on something that is already a JSON
> -- there seems to be no reason for that, it complicates things and does not
help
> performance; another is more a knee-jerk reflex to keep things consistent with
> existing code, though I'm not sure why we originally chose to send stacks as
> separate events (easy to control stack collection via a separate tracing
> category, perhaps?). So I don't really insist on the latter, but let's hear
from
> yurys@ as he picked the existing way to send stacks in a separate event over
> making them a data field.
> 
> Thanks for the explanation. I didn't realize how badly we were handling our
> JSON! In the latest patch I've switched how our stacks are output to store the
> raw JSON and I've aligned the names to match the current inspector events.
> 
> I think you are correct about why the inspector uses two events: so it can be
> controlled separately using the "Causes" (formerly "Stacks") checkbox. I would
> prefer not to send a second event for all of our tracing locations (there are
> about 10) because it clutters the c++ side, especially for our more complex
> trace macros.

Collecting stack traces is notoriously slow and caused complains about Timeline
performance in the past. So we need a way to disable stack trace recording and
do only lightweight Timeline instrumentation. Using a separate category for that
was a natural solution when we switched to tracing. Alternatively we could use
TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED to check whether call stack
collection is enabled and based on that write or not current callstack into
corresponding event's data.

Having said that I don't think it makes to disable call stacks for the
invalidation tracking and having call stacks passed as events arguments should
be fine. I don't like that we will now have two different ways of writing call
stacks for Timeline but we still want to disable them sometimes for things like
setTimeout, rAF etc. It might make sense to change existing instrumentation
using the approach I described above so that call stack is always sent in the
event arguments.

caseq

lgtm https://codereview.chromium.org/654013003/diff/60001/Source/devtools/front_end/timeline/TracingTimelineModel.js File Source/devtools/front_end/timeline/TracingTimelineModel.js (right): https://codereview.chromium.org/654013003/diff/60001/Source/devtools/front_end/timeline/TracingTimelineModel.js#newcode1196 Source/devtools/front_end/timeline/TracingTimelineModel.js:1196: var effectivePaintId = this._lastPaintWithLayer.args.data.nodeId; args["data"]["nodeId"] https://codereview.chromium.org/654013003/diff/60001/Source/devtools/front_end/timeline/TracingTimelineUIUtils.js File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js ...

6 years, 2 months ago (2014-10-17 15:48:55 UTC) #12

pdr.

Thank you all for the reviews! Off to teh CQ https://codereview.chromium.org/654013003/diff/60001/Source/devtools/front_end/timeline/TracingTimelineModel.js File Source/devtools/front_end/timeline/TracingTimelineModel.js (right): https://codereview.chromium.org/654013003/diff/60001/Source/devtools/front_end/timeline/TracingTimelineModel.js#newcode1196 ...

6 years, 2 months ago (2014-10-20 21:38:05 UTC) #14