1844470 - mozilla::ContentEventHandler::GetTextLength is still taking a lot of time on sp3 TipTap

Status: NEW

(Core :: DOM: Editor, defect, P2)

Description

[Markus Stange [:mstange]]

Follow-up to bug 1838355.

Even with the changes from bug 1837482 and bug 1838355, the calls to ContentEventHandler::GetTextLength remain the major bottleneck on the "Editor-TipTap" subtest of Speedometer 3.

Node.insertBefore is 12x slower than in Chrome because of it.
Firefox: https://share.firefox.dev/43DZn2N
Chrome: https://share.firefox.dev/3rA2n31

Node.removeChild is 9x slower than in Chrome because of it.
Firefox: https://share.firefox.dev/44T4PQm
Chrome: https://share.firefox.dev/44LXl1y

Eliminating these performance differences would improve our score on this subtest by 7.2%.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, most time spends in ContentIteratorBase due to its members are strong pointers. It could be fixable with making the class a template.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Trying: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=3368e7a85ac6aa9e93f5dea7dd140407eaf85c6f&newProject=try&newRevision=9c38786068b11564a8202cd8fe93a72071347c79&page=1&framework=13

Comment 3

[Markus Stange [:mstange]]

The patch from that try push does help, good!

It reduces the time spent in the two functions by 1.4x, which is a third of the difference.
insertBefore before patch: ~3500 samples
insertBefore after patch: ~2500 samples
insertBefore in Chrome: ~250 samples

So the patch shaves off 1000 samples of the original difference of ~3000 samples, and should improve our numbers on the subtest by 2.4% (which is one third of 7.2%).

Here's a before/after report from the try push.

But Chrome doesn't appear to call anything like GetFlatTextLengthInRange synchronously during the DOM mutation, so I wonder if we can do something similar.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your check. I'll post the patch to a separate bug.

But Chrome doesn't appear to call anything like GetFlatTextLengthInRange synchronously during the DOM mutation, so I wonder if we can do something similar.

It's hard to say. We could put it off until sending the notification. However, once the DOM is updated during the tick, IME will receive wrong notifications and that leads their crash bugs.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I fixed the blocker bugs so need to reconsider whether this is still valid.

Comment 6

[Markus Stange [:mstange]]

It's better but still a long ways off from Chrome. insertBefore and removeChild are still the top two functions where we are slower than Chrome on Editor-TipTap. Matching Chrome's performance would gain us 3.3% and 2.0% on Editor-TipTap.

Node.insertBefore is now 8.6x slower than in Chrome.
Firefox: https://share.firefox.dev/3qD4Dq7
Chrome: https://share.firefox.dev/3OCDfAC

Node.removeChild is now 6.5x slower than in Chrome.
Firefox: https://share.firefox.dev/3OxXcbI
Chrome: https://share.firefox.dev/45qB21K

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you.

So as smaug guessed, it takes a lot of time in converting nsAtom* to nsHTMLTag in both ContentEventHandler and ContentIterator.

Comment 8

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

it takes a lot of time in converting nsAtom* to nsHTMLTag

I wonder if we could use an approach like this one in Chrome where they switch on the length first, then on the first character, and then do some memcmps for the remainder.

Comment 9

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #7)

it takes a lot of time in converting nsAtom* to nsHTMLTag

Bug 1849204 will probably improve the current hash table lookup, by decreasing the hashing cost. But the PLDHashTable::SearchTable part would probably still remain, so the idea from the previous comment might still be worth trying.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, if we'd take the approach, it seems that we don't need TagAtomHash anymore...