Summary: | Timeline Panel timings are polluted by DOM Mutation events (which force inline Style Recalculations) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jaimeyap | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Kelly Norton <knorton> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, adeveria, dglazkov, jamesr, knorton, pfeldman, timothy, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
jaimeyap
2009-12-21 19:00:03 PST
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. We could not use mutation events and hard-wire InspectorController to be notified when things mutate. 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. 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. 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 Yes, most likely that can be fixed as well. But that's a larger, trickier issue. (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)? I suppoert 2, mainly for Attr mutations and muting the Elements panel is weird to me. 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.
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
Attachment 46006 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/164985 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. > 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!
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. 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?)
style-queue ran check-webkit-style on attachment 46083 [details] without any errors.
Attachment 46083 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/166566 > 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?
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. :-) Created attachment 46088 [details]
Fixes the chromium build issue?
style-queue ran check-webkit-style on attachment 46088 [details] without any errors.
*** Bug 38751 has been marked as a duplicate of this bug. *** |