Deleting frames under nsCSSFrameConstructor::ContentRemoved() can be really expensive

RESOLVED FIXED

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: jwatt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:meta], )

Reporter

Description

2 years ago
See bug 1363335.  There is a lot of cost we're paying there under an innerHTML setter just deleting frames, and it's mostly hashtable lookups and allocator overhead, so it seems like we should be able to do a lot better...

Jet, can you please find an owner?  This is a Quantum Release Criteria blocker.
Flags: needinfo?(bugs)
Reporter

Comment 1

2 years ago
Link to profile: https://perfht.ml/2rgD0T5
Reporter

Updated

2 years ago
Jonathan: please let us know if your HashTable changes will help this case, and also if this is another case of free() being an expensive call in some cases. Thx!
Flags: needinfo?(bugs) → needinfo?(jfkthame)
It looks like we have several hashtables that get involved here. I'm hoping to eliminate the PresContext's FramePropertyTable completely, so that may help somewhat, but we also have nsPropertyTable on the dom side, and the FrameManager has its UndisplayedMap hashes; I'm not currently touching those.

I hope to have a testable patch in bug 1365982 pretty soon, so then we can re-profile and see how much things change.
Flags: needinfo?(jfkthame)
Depends on: 1365982
Link to a slightly more fine-grained profile: https://perfht.ml/2qTwGOC
Assigning this to jwatt for now. He'll re-profile after landing the fix for bug 1367214.
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Priority: -- → P1
Calling this [qf:meta] since it seems the work here is all happening in dependent bugs.
Whiteboard: [qf:p1] → [qf:meta]
Assignee

Comment 7

2 years ago
Ehsan, all this bugs blockers are now fixed. Since you filed this bug, is there anything else you've seen that would warrant keeping this bug open?
Flags: needinfo?(ehsan)
Reporter

Comment 8

2 years ago
No, I think we can close this, thanks!  Also not sure why this bug was marked as "stale" or what that means, but clearing that keyword since that seems to be misguided.
Flags: needinfo?(ehsan)
Keywords: stale-bug
Reporter

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.