Closed Bug 1385384 Opened 7 years ago Closed 7 years ago

Don't store mPlaceholderTransactionWeak as a weak pointer

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Weak pointers are needlessly expensive, and they show up in profiles, for example see https://perfht.ml/2v75r7A.

The reason why this used to be a weak pointer was that this code goes back to before we had a cycle collector, but with a cycle collector this doesn't need to be a weak pointer any more at all, and can just be a normal strong pointer.  This way we can avoid the extra cost of dynamically allocating the proxy object.
This patch alone shaves off a couple of milliseconds from the test case of bug 1346723 for me locally.
Comment on attachment 8891451 [details] [diff] [review]
Don't store mPlaceholderTransactionWeak as a weak pointer

Interesting. I don't understand about Cycle Collector, though, how about other weakptr members like mSelectionControllerWeak and mDocumentWeak? If they don't need to be weak pointers, we can reduce the runtime cost for them too.
Attachment #8891451 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> Comment on attachment 8891451 [details] [diff] [review]
> Don't store mPlaceholderTransactionWeak as a weak pointer
> 
> Interesting. I don't understand about Cycle Collector, though, how about
> other weakptr members like mSelectionControllerWeak and mDocumentWeak? If
> they don't need to be weak pointers, we can reduce the runtime cost for them
> too.

Yes, those can probably be converted into strong pointers in a very similar way.  The cost of mSelectionControllerWeak definitely shows up through GetSelection() in profiles.  I may get to those later...
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/780d578d1f74
Don't store mPlaceholderTransactionWeak as a weak pointer; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/780d578d1f74
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.