1830542 - nsRange::RegisterSelection shows up in sp3/TodoMVC-Svelte Adding100Items.sync profiles

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

TodoMVC-Svelte Adding100Items.sync spends some time in creating SelectionListWrapper under nsRange::RegisterSelection.
https://share.firefox.dev/425XlIB

Could we recycle SelectionListWrapper objects, or avoid creating them in the first place or something else...

https://speedometer-preview.netlify.app/interactiverunner

Comment 1

[Jan Jaeschke [:jjaschke]]

From the top of my head I see two options:

• Recycle the instances, just as we do with nsRange
• Change the container from mozilla::LinkedList<RefPtr<SelectionListWrapper>> to std::list<RefPtr<Selection>>, the wrapper only exists because of the limitations/design choices of mozilla::LinkedList in the first place.

If std::list is allowed in mozilla codebase, I'd prefer that solution. Then we can get rid of the wrapper class, and it has reduced complexity to adding some cache of instances... WDYT?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why std::list?
LinkedList or DoublyLinkedList should work, no?

Comment 3

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1830542: Avoid unnecessary allocation when registering `Selection`s in `nsRange`s. r=smaug!

nsRanges need to keep track of all Selection instances they are in, while maintaining an as-small-as-possible memory footprint.

The approach of using a linked list to store the selection pointers led to a performance regression
because of the necessary allocations of the selection wrapper class.

Since the AutoTArray has an identical size, the list can easily be replaced.

Comment 4

[Jan Jaeschke [:jjaschke]]

Re: Comment #1, I opted for option 3: Replace the linked list with an AutoTArray<, 1>, which does not allocate anything for >99% of cases (i.e., only if a range is part of two or more selections) while having the same memory footprint of the LinkedList. This also removes the need of the SelectionListWrapper class.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a16fb734776
Avoid unnecessary allocation when registering `Selection`s in `nsRange`s. r=smaug

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3a16fb734776