657724 - Stop sending BeginUpdate/EndUpdate on attribute changes

Status: NEW

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Gah.  There are also frame AttributeChanged implementations that mutate the frame tree (!).

I'll need to audit those....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Specifically, nsTableCellFrame::AttributeChanged calling nsTableFrame::AttributeChangedFor which changes the cellmap and then starts messing with colframes.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Not sure whether this has been coming up noticeably in quantum flow profiles...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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%)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make sure to MaybeBroadcast on AttributeChanged in XULBroadcastManager, so that we're not depending on the MaybeBroadcast from EndUpdate for attribute changes.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch attribute changes away from calling Begin/EndUpdate.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a conditional script blocker that only adds/removes script blockers if needed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: VERY BITROTTED part 4. Switch inline style changes away from calling Begin/EndUpdate.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.