1925078 - 3.73 - 1.38% speedometer Vanilla-ES2015-TodoMVC/Adding100Items/Sync / speedometer Vanilla-ES2015-TodoMVC/Adding100Items + 1 more (Linux) regression on Tue September 24 2024

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Treeherder Bug Filer]

Perfherder has detected a browsertime performance regression from push 87f6de51f322e7c2feee21224dd083de8d2a3d98. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	speedometer Vanilla-ES2015-TodoMVC/Adding100Items/Sync	linux1804-64-nightlyasrelease-qr	fission webrender	52.49 -> 54.45	Before/After
3%	speedometer Vanilla-ES2015-TodoMVC/Adding100Items	linux1804-64-nightlyasrelease-qr	fission webrender	55.84 -> 57.62	Before/After
1%	speedometer Vanilla-ES2015-TodoMVC/Adding100Items	linux1804-64-nightlyasrelease-qr	fission webrender	55.73 -> 56.50	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 2410

The following documentation link provides more information about this command.

For more information on performance sheriffing please see our FAQ.

If you have any questions, please do not hesitate to reach out to [e-mail].

Comment 1

[Alex Finder]

Just to clarify, this alert summary is for revision 0f470e1a67afe6044b03ba5c8dd92daa3037de8a. Creating a new alert on that revision would have classified it as an improvement instead of a regression. Please add a needinfo if there are further question regarding this alert. Thanks!

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1919927

Comment 3

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

Hmm, nsGenericHTMLElement::IsContentEditable stops inlined, but it's called only when HTMLEditor receives user inputs. So, I think this is not related to this performance issue.

I guess this is a pure performance issue of nsGenericHTMLElement::GetContentEditableState which starts checking the new value, plaintext-only.

Comment 4

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

If I remove the plaintext-only value check from GetContentEditableState, the score is completely recovered. So, the slowdown must be the necessary cost to implement contenteditable="plaintext-only". On the other hand, a lot of calls of nsAttrValue::Equals could be avoided. I'll keep trying to optimize the method.

Comment 5

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

Perhaps GetContentEditableState should use FindAttrValueIn, like GetContentEditableValue used to.
Maybe the param for FindAttrValueIn could depend on the plaintext-only pref?

(Another option might be to use ElementState flags or nsINode flags.)

Comment 6

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

Thank you for the information. However, FindAttrValueIn calls nsAttrValue::Equals for each candidate value. So, I guess that it does not improve the performance unless the compiler optimizes the code differently.

(Anyway, it's difficult to test the score due to 1% is really small difference to get the result only with 20 - 30 runs.)

Comment 7

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

Attachment: Bug 1925078 - Optimize `nsGenericHTMLElement::GetContentEditableState` and make `nsGenericHTMLElement::UnbindFromTree` avoid to check the `contenteditable` value as far as possible r=smaug!

The value comparison of contenteditable is not cheap due to the possible value
is "", "true", "plaintext-only" and "false" which are not case
sensitive. However, nsGenericHTMLElement::UnbindFromTree may be called a lot
and it needs to notify document of the removal of a (potential) editing host.

nsGenericHTMLElement::GetContentEditableState was optimized with a bool flag
indicating "may have contenteditable attribute". However, we can improve it
with replacing it with 2 bool flags. One is set if "may have contenteditable
attribute whose value may make it an editing host" and "may have contenteditable
attribute whose value is false".

This patch sets both flags when BeforeSetAttr is called for
contenteditable attribute even if it's being removed because when it's called,
the value depends on when it's referred (before or after the mutation).
Additionally, this patch clears unnecessary flags when AfterSetAttr is called.
When AfterSetAttr is called, we need to check the new value anyway.
Therefore, we can correct the flags only with clearing flag cost. Then,
GetContentEditableState can trust the flags unless both of them are set.

Additionally, UnbindFromTree may be a host path and it requires to check
whether it was an editing host or not. Therefore, this patch adds a new method
which does not check "false" value.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fdc84b66933
Optimize `nsGenericHTMLElement::GetContentEditableState` and make `nsGenericHTMLElement::UnbindFromTree` avoid to check the `contenteditable` value as far as possible r=smaug

Comment 9

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/6fdc84b66933

Comment 10

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox133 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

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

Let's consider it after getting the perf alert because that must contain a performance regression with a lot of improvements.

Comment 12

[Donal Meehan [:dmeehan]]

Setting Fx133 to wontfix.
It's not a new bug in Fx133 and the patches look like they should ride the trains.