Closed Bug 1340771 Opened 3 years ago Closed 3 years ago

consider using a better data structure for weak frames

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: tnikkel, Assigned: mats)

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).
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...
Assignee: nobody → mats
Keywords: perf
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8841592 - Flags: review?(tnikkel)
Attachment #8841593 - Flags: review?(tnikkel)
Attachment #8841594 - Flags: review?(tnikkel)
Attachment #8841595 - Flags: review?(tnikkel)
This looks good. Would it be clearer to name them StackWeakFrame and HeapWeakFrame?
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+
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.
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+
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+
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+
(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.
(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.
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
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)
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.
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.
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)
You don't need this. Just drop the servo/ changes from your patch and land it.
Flags: needinfo?(simon.sapin)
OK, thanks.  FYI, it looks like linux64-servo compiled fine on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d6463c2a5c4b848c6f86606b8c2522951832f7
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 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)
Attachment #8841592 - Flags: review?(Ms2ger)
Blocks: 1118086
You need to log in before you can comment on or make changes to this bug.