Open
Bug 657724
Opened 13 years ago
Updated 3 months ago
Stop sending BeginUpdate/EndUpdate on attribute changes
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
NEW
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 4 open bugs)
Details
(Keywords: perf)
Attachments
(4 files)
655 bytes,
patch
|
Details | Diff | Splinter Review | |
2.32 KB,
patch
|
Details | Diff | Splinter Review | |
1.47 KB,
patch
|
Details | Diff | Splinter Review | |
10.16 KB,
patch
|
Details | Diff | Splinter Review |
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?
![]() |
Reporter | |
Comment 1•13 years ago
|
||
Gah. There are also frame AttributeChanged implementations that mutate the frame tree (!). I'll need to audit those....
![]() |
Reporter | |
Comment 2•13 years ago
|
||
Specifically, nsTableCellFrame::AttributeChanged calling nsTableFrame::AttributeChangedFor which changes the cellmap and then starts messing with colframes.
![]() |
Reporter | |
Comment 3•13 years ago
|
||
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.
![]() |
Reporter | |
Comment 4•13 years ago
|
||
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.
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
![]() |
Reporter | |
Comment 5•7 years ago
|
||
Not sure whether this has been coming up noticeably in quantum flow profiles...
Comment 6•7 years ago
|
||
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%)
![]() |
Reporter | |
Comment 7•7 years ago
|
||
> 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.
Updated•6 years ago
|
Priority: P1 → P3
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
![]() |
Reporter | |
Comment 8•4 years ago
|
||
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 → --
![]() |
Reporter | |
Comment 9•4 years ago
|
||
![]() |
Reporter | |
Comment 10•4 years ago
|
||
![]() |
Reporter | |
Comment 11•4 years ago
|
||
![]() |
Reporter | |
Comment 12•4 years ago
|
||
![]() |
Reporter | |
Updated•4 years ago
|
Attachment #9132069 -
Attachment is patch: true
![]() |
Reporter | |
Comment 13•4 years ago
|
||
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.
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•