653887 - Investigate changing the begin/end update contract

Status: NEW

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we require BeginUpdate/EndUpdate on all mutation/document observer notifications.

I think we should drop this requirement.  In particular, I think we should start by dropping it for state change notifications and attribute modifications.  I don't think those are really batched effectively anyway; the few consumers that do more mutation under the attribute notification (e.g. XBL) can do it off a script runner.

Once we no longer flush sinks from BeginUpdate for insertion/removal of nodes we can think about removing those begin/ends as well.

We would need to double-check the existing Begin/EndUpdate implementations to make sure they're ok with this change, of course, and we may still need scriptblockers for attribute changes, at least.

The win from doing this is saving lots of code running per mutation; for example for inline style changes the begin+end for the attr change is somewhere in the 30-50% range of the total time for the inline style change.

Anything obviously wrong with this plan?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yay! Lets do it! I'll try to do the XBL parts as part of bug 653881, or as a followup to it.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We would need to change how we manage nsBindingManager::mAttachedStackSizeOnOutermost since that currently relies on BeginUpdate/EndUpdate and we'd want to move it to a scriptrunner which doesn't have the begin part.

We'd have to do that even if we're just changing attribute changes since they can cause bindings to be added/removed.

However I think it should work if ProcessAttachedQueue sets mAttachedStackSizeOnOutermost to the current mAttachedStack.Length() while running every attach handler. Or something like that :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So we have the following implementations of nsIDocumentObserver::BeginUpdate:

1) PresShell
2) XMLPrettyPrinter
3) nsContentSink

That's it.  Plus whatever nsDocument::Begin/EndUpdate do.

The nsContentSink ones already ignore UPDATE_CONTENT_STATE and will go away with Henri's XML work anyway.  It also doesn't actually care about attribute changes; it just can't tell them apart from DOM insertions/removals.

The XMLPrettyPrinter just wants to know when the last update ends so it can drop its stuff.  This can happen off a script runner as long as script blockers are still installed at current begin/end callsites (which we want to do).

Presshell is more interesting.  It calls through to the frame constructor and for UPDATE_STYLE updates batches style set changes.  That batching is actually useful; I think we will want to keep it.  Luckily, UPDATE_STYLE changes are rare and already expensive, so that would be OK.

Frame constructor needs to detect ends of outermost updates so it can recalc quotes and counters.  It also calls IncrementDOMGeneration() on the prescontext, which is used to bail out of FrameLayerBuilder::DrawThebesLayer if the DOM changes under it.  We should just do that on actual mutation notifications; we certainly don't need to do it on state changes.  As for quotes and counters, the quotes stuff only cares about frames being destroyed and created.  Same for counters.  Neither attribute nor state changes on their own can cause that to happen.

nsDocument does the mInXBLUpdate thing Jonas promised to remove, which is all about notifying the binding manager.  It also handles adding/removing script blockers (removable for UPDATE_CONTENT_MODEL, not removable otherwise) and notifying the three observers above.

The binding manager just needs to keep track of the attached stack depth, so it's only concerned about things that can cause binding attachment.  I believe neither attribute nor state changes can trigger that directly.

So I think that we can remove the begin/end stuff for attributes and states right now, if we move prettyprinter to a scriptrunner.  For insert/remove we'll need to do something with binding manager, the sink, and the frame constructor.  STYLE updates we want to keep the begin/end for.

Any obvious errors in the above?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> attribute changes since they can cause bindings to be added/removed

Synchronously under the attribute change update (ignoring mutation events for now)?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Talked to roc; he thinks IncrementDOMGeneration should be safe for state and attr changes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  I'm going to file separate bugs for the state thing (which I need right this second) and the attribute thing (which I will do ASAP).