Closed
Bug 1340771
Opened 7 years ago
Closed 7 years ago
consider using a better data structure for weak frames
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: tnikkel, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(4 files)
Weak frames are stored in a linked list, so whenever a weak frame goes away or a frame goes away we have to search the entire list of weak frames. We have a fair number of nsWeakFrame fields in classes (as opposed to just local variables in functions) so they could be relatively long lived and we could get many weak frames alive at once. Bug 1118086, comment 18 is one example of this (since fixed, but not all case are as easily fixed).
Assignee | ||
Comment 1•7 years ago
|
||
It's hard to beat a LIFO linked list in speed for the stack-based objects though. Perhaps it would be best to have two separate classes: AutoWeakFrame for stack-only objects which is continued to be stored as a LIFO linked list, and WeakFrame for non-stack uses that is stored in a hashtable or something (and thus doesn't need to have a mPrev member). I think this would be fairly easy to implement: 1. rename nsWeakFrame to AutoWeakFrame and mark it MOZ_RAII 2. implement the new WeakFrame class, mark it MOZ_HEAP_CLASS 3. switch non-stack objects over to WeakFrame I'll take a stab at this...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8841592 -
Flags: review?(tnikkel)
Assignee | ||
Updated•7 years ago
|
Attachment #8841593 -
Flags: review?(tnikkel)
Assignee | ||
Updated•7 years ago
|
Attachment #8841594 -
Flags: review?(tnikkel)
Assignee | ||
Updated•7 years ago
|
Attachment #8841595 -
Flags: review?(tnikkel)
Reporter | ||
Comment 6•7 years ago
|
||
This looks good. Would it be clearer to name them StackWeakFrame and HeapWeakFrame?
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8841593 [details] Bug 1340771 part 2 - Introduce a WeakFrame class for heap allocated weak frame pointers, stored in a hashtable for fast lookup. https://reviewboard.mozilla.org/r/115740/#review117262 ::: layout/base/nsIPresShell.h:1209 (Diff revision 1) > AddWeakFrameInternal(aWeakFrame); > #else > AddWeakFrameExternal(aWeakFrame); > #endif > } > + void AddWeakFrame(WeakFrame* aWeakFrame) Can we insert Heap and/or Auto (or Stack) into the function names of the functions on the presshell for clarity? ::: layout/generic/nsIFrame.h:3910 (Diff revision 1) > + MOZ_IMPLICIT WeakFrame(const AutoWeakFrame& aOther) : mFrame(nullptr) > + { > + Init(aOther.GetFrame()); > + } > + > + MOZ_IMPLICIT WeakFrame(nsIFrame* aFrame) : mFrame(nullptr) I was a little surprised that our existing weak frame allowed implicit construction from an nsIFrame. I think I would prefer conversion to/frame weakframes to be explicit.
Attachment #8841593 -
Flags: review?(tnikkel) → review+
Reporter | ||
Comment 8•7 years ago
|
||
I guess there is still one potential performance problem with weak frames after this patch: destroying nsIFrame's that have weak frames tracking them could be slow. But that is not this bug.
Reporter | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8841592 [details] Bug 1340771 part 1 - Rename nsWeakFrame to AutoWeakFrame (automated change). https://reviewboard.mozilla.org/r/115738/#review117266
Attachment #8841592 -
Flags: review?(tnikkel) → review+
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8841594 [details] Bug 1340771 part 3 - Change existing heap allocated AutoWeakFrame instances to use WeakFrame instead. https://reviewboard.mozilla.org/r/115742/#review117268
Attachment #8841594 -
Flags: review?(tnikkel) → review+
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8841595 [details] Bug 1340771 part 4 - Mark AutoWeakFrame as MOZ_NONHEAP_CLASS and WeakFrame as MOZ_HEAP_CLASS. https://reviewboard.mozilla.org/r/115744/#review117270
Attachment #8841595 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #6) > This looks good. Would it be clearer to name them StackWeakFrame and > HeapWeakFrame? The reason I chose AutoWeakFrame/WeakFrame is that it's an already established convention: nsAutoString/nsString, AutoTArray/nsTArray etc. So I think I prefer to stick to that.
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #7) > Can we insert Heap and/or Auto (or Stack) into the function names of the > functions on the presshell for clarity? I renamed the ones taking a AutoWeakFrame so the method names now follow the same naming convention. > > + MOZ_IMPLICIT WeakFrame(nsIFrame* aFrame) : mFrame(nullptr) > > I was a little surprised that our existing weak frame allowed implicit > construction from an nsIFrame. > > I think I would prefer conversion to/frame weakframes to be explicit. I just followed the existing code there. I don't see a problem with it myself, but perhaps I'm missing something. We can handle that in a follow-up bug though if there's a specific use case you're concerned about.
Assignee | ||
Comment 14•7 years ago
|
||
Sigh: remote: (b4f7c3141263 modifies servo/components/style/gecko_bindings/structs_debug.rs from Servo; not enforcing peer review) remote: (b4f7c3141263 modifies servo/components/style/gecko_bindings/structs_release.rs from Servo; not enforcing peer review) remote: (1 changesets contain changes to protected servo/ directory: b4f7c3141263) remote: ************************************************************************ remote: you do not have permissions to modify files under servo/ remote: remote: the servo/ directory is kept in sync with the canonical upstream remote: repository at https://github.com/servo/servo
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8841592 [details] Bug 1340771 part 1 - Rename nsWeakFrame to AutoWeakFrame (automated change). Could anyone who is a servo peer please review the changes under servo/ in this patch please? Or tell me if I should land without those changes?
Attachment #8841592 -
Flags: review?(simon.sapin)
Attachment #8841592 -
Flags: review?(Ms2ger)
Comment 16•7 years ago
|
||
Unfortunately, the automatic repository synchronization is not fully in place yet. I think that you need to re-apply the same changes in the https://github.com/servo/servo/ repository and send a GitHub pull request. Once you do that I can approve it, CI will run tests, merge into master, and these changes will automatically be imported into integration/autoland.
Comment 17•7 years ago
|
||
Do you actually need to modify this in the checked-in bindings? If linux64-stylo compiles fine without it, you can just ignore it. Actual stylo builds generate bindings at build-time, and the checked-in bindings are just so that we can keep our library compiling on Servo CI.
Assignee | ||
Comment 18•7 years ago
|
||
Wow, that's ... inconvenient. ;-) Does this look right: https://github.com/servo/servo/pull/15785 Do I need to add the new field I added in part 2 as well? (nsTHashtable<nsPtrHashKey<WeakFrame>> mHeapWeakFrames)
Flags: needinfo?(simon.sapin)
Comment 19•7 years ago
|
||
You don't need this. Just drop the servo/ changes from your patch and land it.
Flags: needinfo?(simon.sapin)
Assignee | ||
Comment 20•7 years ago
|
||
OK, thanks. FYI, it looks like linux64-servo compiled fine on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d6463c2a5c4b848c6f86606b8c2522951832f7
Comment 21•7 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/75caefd9df14 part 1 - Rename nsWeakFrame to AutoWeakFrame (automated change). r=tn https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8eb0ca05ac part 2 - Introduce a WeakFrame class for heap allocated weak frame pointers, stored in a hashtable for fast lookup. r=tn https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf51d656aef part 4 - Mark AutoWeakFrame as MOZ_NONHEAP_CLASS and WeakFrame as MOZ_HEAP_CLASS. r=tn
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8841592 [details] Bug 1340771 part 1 - Rename nsWeakFrame to AutoWeakFrame (automated change). https://reviewboard.mozilla.org/r/115738/#review118000
Attachment #8841592 -
Flags: review?(simon.sapin)
Assignee | ||
Updated•7 years ago
|
Attachment #8841592 -
Flags: review?(Ms2ger)
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75caefd9df14 https://hg.mozilla.org/mozilla-central/rev/cb8eb0ca05ac https://hg.mozilla.org/mozilla-central/rev/3cf51d656aef
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•