Closed Bug 657724 Opened 13 years ago Closed 9 days ago

Stop sending BeginUpdate/EndUpdate on attribute changes


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






(Reporter: bzbarsky, Unassigned)


(Blocks 3 open bugs)


(Keywords: perf, Whiteboard: [domcore-bugbash-triaged])


(4 files)

It's slow, we don't need it.  See bug 653887.

But actually, I think this would regress bug 423269 because we'd get Begin/EndUpdate happening under a scriptblocker with no outer updates....  Jonas, is that correct?  Do we need to move more things that listen for EndUpdate to post scriptrunners in EndUpdate, perhaps?
Gah.  There are also frame AttributeChanged implementations that mutate the frame tree (!).

I'll need to audit those....
Specifically, nsTableCellFrame::AttributeChanged calling nsTableFrame::AttributeChangedFor which changes the cellmap and then starts messing with colframes.
Depends on: 653126
And of course XBL does the whole add/remove nodes thing under nsXBLPrototypeBinding::AttributeChanged...

On the other hand, the dep on bug 653126 is the other way: if we stop calling BeginUpdate right up front in SetAttrAndNotify (and specifically if we don't call it anywhere before the SetAndTakeAttr call), then the problem of accidentally flushing the sink when our style data is not in a good state is gone.  So fixing this bug would automagically fix bug 653126.
Blocks: 653126
No longer depends on: 653126
I further believe that the add/remove I mention in comment 3 is fine, except insofar as it triggers outermost EndUpdate while still under a scriptblocker.
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 664818
Not sure whether this has been coming up noticeably in quantum flow profiles...
Not too much, perhaps because RestyleManager's AttributeWillChange/AttributeChanged are so slow that
one may not even end up looking at Begin/EndUpdate so closely.
Or in innerHTML cases SetAttr happens before element is in tree so everything is reasonable fast.

I did file bug 1350885 since EndUpdate shows up a bit when inserting new elements to tree, so I guess same applies to SetAttr too.
(EndUpdate was showing up more before bug 1352389)
Hmm, but is this bug about nsIDocumentObserver's method, or the Begin/EndUpdate on nsDocument?
If the former, then this has shown up even less. I guess this is about that.

In a synthetic testcase nsDocument::EndUpdate does seem to take 3% of attribute setting.
(RestyleManager's AttributeWillChange/AttributeChanged together >22%)
> Hmm, but is this bug about nsIDocumentObserver's method, or the Begin/EndUpdate on nsDocument?

Both.  The latter calls the former, of course.  All the complicated logic that makes it hard to say whether it's ok to skip this is in the former.
Priority: P1 → P3
Component: DOM → DOM: Core & HTML

I didn't really get anywhere with this, unfortunately. I'll see if I can post the old patches that I had related to this, at least...

Assignee: bzbarsky → nobody
Priority: P3 → --
Attachment #9132069 - Attachment is patch: true

That style patch would need unbitrotting, and likely the previous patches have bitrot too, just not bitrot that shows up when trying to apply them anymore; I merged them to tip that much...

And the bigger questions about whether this is doable are still open.

Severity: normal → S3

Performance has been improved heavily recently, including nsTableFrame code etc.
If there are still issues here, we should file new bugs with profiles etc.

Whiteboard: [domcore-bugbash-triaged]
Closed: 9 days ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.