Bug 32846 – Timeline Panel timings are polluted by DOM Mutation events (which force inline Style Recalculations)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

When inspecting a page, the InspectorController binds the DOMAgent, which from my understanding registers DOM mutation event listener. This causes DOM Mutation Events to fire for every DOM mutation (innerHTML parsing, dom node appends, document fragment appends, etc...).

This gets compounded by the fact that whenever a JS event handler for a DOM event is fired at a document context a call to Document::updateStyleForAllDocuments() is made. Thus firing DOM Mutation events repeatedly forces styles to be up to date inline with each mutation instead of getting deferred.

see https://bugs.webkit.org/show_bug.cgi?id=32580 which is Jamesr's pending attempt to address the inline recalculation of styles.

But a proper fix would be to somehow disable DOMMutation event dispatches when the timeline agent is recording.

Essentially, timing information on the timeline panel can be totally incorrect for certain traces.

(In reply to comment #0)
> When inspecting a page, the InspectorController binds the DOMAgent, which from
> my understanding registers DOM mutation event listener. This causes DOM
> Mutation Events to fire for every DOM mutation (innerHTML parsing, dom node
> appends, document fragment appends, etc...).
> 
> This gets compounded by the fact that whenever a JS event handler for a DOM
> event is fired at a document context a call to
> Document::updateStyleForAllDocuments() is made. Thus firing DOM Mutation events
> repeatedly forces styles to be up to date inline with each mutation instead of
> getting deferred.
> 
> see https://bugs.webkit.org/show_bug.cgi?id=32580 which is Jamesr's pending
> attempt to address the inline recalculation of styles.
> 
> But a proper fix would be to somehow disable DOMMutation event dispatches when
> the timeline agent is recording.

[Timothy Hatcher]

We could not use mutation events and hard-wire InspectorController to be notified when things mutate.

[Pavel Feldman]

We can temporarily disable DOMAgent, visually disable Elements panel as a whole while timeline is running. Once it has completed, we can re-push DOM tree and restore elements tree selection. Seems to be fairly straightforward and local change.

[James Robinson]

Unregistering the DOM mutation event listeners won't change anything - setting them sets bits on the Document's m_listenerTypes bitset which causes the mutation events to fire.  See http://trac.webkit.org/browser/trunk/WebCore/dom/Document.h#L616. These bits are never cleared, so the event dispatch code will still run (including the style match) even if the DOM panel is disabled.

[Pavel Feldman]

Isn't it something we should fix anyways? Like putting counters for all the types does not sound like a huge overhead. Especially given the way addListenerType is dispatched: http://www.google.com/codesearch/p?hl=en#N6Qhr5kJSgQ/WebCore/dom/Document.cpp&q=addListenerTypeIfNeeded&sa=N&cd=1&ct=rc&l=2791

[James Robinson]

Yes, most likely that can be fixed as well.  But that's a larger, trickier issue.

[Pavel Feldman]

(In reply to comment #6)
> Yes, most likely that can be fixed as well.  But that's a larger, trickier
> issue.

So we have two options:

1) Mute elements panel, implement removeListenerType.

- Adding removeListenerType sounds straightforward to me - I can only see addListenerType invoked in two places: DOMWindow and Node. Both have symmetric remove placeholders. Seems like a good thing in general, but I know that people do not remove listeners, so we are not likely to gain much WebCore performance gain out of it.
- Muting dom panel is an extra effort that will complicated ui and I'd like to avoid it if possible.

2) Push dom events explicitly into inspector controller.

DOMNodeInsertedEvent, DOMNodeRemovedEvent, DOMAttrModifiedEvent, DOMContentLoadedEvent and loadEvent should be pushed into InspectorController for all the documents on the page. InspectorController should then decide whether it needs them or not.
This approach is interesting for two reasons: 
- we can get these events for detached subtrees.
- we could get Attr events as a side effect

Tim, are you supporting (2)?

[Timothy Hatcher]

I suppoert 2, mainly for Attr mutations and muting the Elements panel is weird to me.

[Kelly Norton]

Created attachment 46006 [details]
Removes DOM mutation listeners and adds direct calls to inspector.

Couple of notes about this patch:

(1) It leaves the listeners for load and DOMContentLoaded as they were in InspectorDOMAgent. There really is no need to remove these listeners as they don't interfere like mutation events do.

(2) Attr mutations are now pushed into the front-end, but the front-end doesn't have events setup to update the ElementPanel when attributes are mutated. I did not fix this yet. I figured I would just do it after this one goes in.

[WebKit Review Bot]

Attachment 46006 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorDOMAgent.cpp:661:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1

[WebKit Review Bot]

Attachment 46006 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/164985

[James Robinson]

This should be one line:

InspectorController* inspectorController = page->inspectorController();
if (inspectorController)
    ....


Also, could you run dromaeo DOM bench with and without this patch (with the
inspector off) just to make sure that walking up through the page and inspector
pointers doesn't slow anything down?  This codepath is super duper hot.

[Pavel Feldman]

> Also, could you run dromaeo DOM bench with and without this patch (with the
> inspector off) just to make sure that walking up through the page and inspector
> pointers doesn't slow anything down?  This codepath is super duper hot.

Yes, we need to make sure we don't regress performance here. We actually need something similar for instrumentation of selected bindings in timeline. Should we add a static counter (counters) in InspectorController that would track whether there in a dom agent / timeline agent interested in the information?

Also, feel free to surround attr mutation event with "if 0" in dom agent code - it has never been getting control prior to your change (or give it some testing and leave it enabled). Otherwise, patch looks fine!

[Kelly Norton]

Performance differences on dromaeo seem insignificant (indistinguishable from noise):

r52875 + patch
http://dromaeo.com/?id=85056
http://dromaeo.com/?id=85058

r52875 (no changes)
http://dromaeo.com/?id=85060
http://dromaeo.com/?id=85062

Pavel, I confirmed that the call to updateAttributes causes no problems (as in no exceptions in the Inspector). It actually updates the list of attributes for a Node appropriately, it's just that there isn't an event to cause the ElementsPanel to update.


I'll fix the style issue and see what's the deal is on Chromium and submit an updated patch.

[Kelly Norton]

Created attachment 46083 [details]
Patch fixes style issues.

I fixed the style issues, but I'm unable to reproduce the Chromium build break. (Is that bot dependable?)

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46083 [details] without any errors.

[WebKit Review Bot]

Attachment 46083 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/166566

[Adam Barth]

> I fixed the style issues, but I'm unable to reproduce the Chromium build break.
> (Is that bot dependable?)

It builds release.  Maybe it's a release / debug issue?

[Kelly Norton]

I think I found the issue. I'm not going to say it was my fault ... but it certainly wasn't the bot's fault. :-)

[Kelly Norton]

Created attachment 46088 [details]
Fixes the chromium build issue?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46088 [details] without any errors.

[James Robinson]

http://trac.webkit.org/changeset/53080

[Dirk Schulze]

*** Bug 38751 has been marked as a duplicate of this bug. ***