Closed Bug 1842027 Opened 1 year ago Closed 1 year ago

Firefox is 45x slower than Chrome at setAttribute on <input type="number">

Categories

(Core :: DOM: Forms, defect)

Product:
Core
Component:
DOM: Forms
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
117 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox-esr115 --- wontfix
firefox115 --- wontfix
firefox116 --- wontfix
firefox117 --- fixed

People

(Reporter: mstange, Assigned: emilio)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [sp3])

Attachments

(8 files)

microbenchmark
(deleted), text/html
Details
Bug 1842027 - Make <input type=number> localization faster. r=smaug,masayuki
(deleted), text/x-phabricator-request
Details
Bug 1842027 - Remove dead nsTextControlFrame::GetText. r=smaug,masayuki
(deleted), text/x-phabricator-request
Details
Bug 1842027 - Remove always-true aIgnoreWrap parameter from TextControlElement::GetTextEditorValue. r=smaug,masayuki
(deleted), text/x-phabricator-request
Details
Bug 1842027 - Remove nsTextControlFrame::TextEquals. r=smaug,masayuki
(deleted), text/x-phabricator-request
Details
Bug 1842027 - Don't localize <input type=number>'s default value unless for display. r=smaug,masayuki
(deleted), text/x-phabricator-request
Details
perf reftest patch (untested)
(deleted), text/plain
Details
Bug 1842027 - Add a perf reftest. r=#perftest-reviewers
(deleted), text/x-phabricator-request
Details
Attached file microbenchmark (deleted) —

On the attached testcase, I get 955ms in Firefox and 21ms in Chrome. https://share.firefox.dev/43eo7yv

Making Firefox as fast as Chrome on this testcase would improve our time on the "Perf-Dashboard" subtest of speedometer3 by 4.5%.

That's kind of ridiculous.

Flags: needinfo?(emilio)

A Nightly from 2020-01-14 actually performs better than Chromium in this case, very interesting

Nightly from 2020-01-14: 24ms
Current Chromium: 29ms
Nightly from 2020-01-15: 1178ms

3:21.82 INFO: Last good revision: 12d8255184b1015ff34f35c6fb040bbde6be2ed3 (2020-01-14)
3:21.82 INFO: First bad revision: c35bb210b8ae793c844bd94c1848d246bf601293 (2020-01-15)
3:21.82 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=12d8255184b1015ff34f35c6fb040bbde6be2ed3&tochange=c35bb210b8ae793c844bd94c1848d246bf601293

Nice find, I'm guessing bug 981248.

WIP - This is still not as fast as it should be (see FIXME).

Assignee: nobody → emilio
Attachment #9343263 - Attachment description: WIP: Bug 1842027 - Make <input type=number> localization faster. → Bug 1842027 - Make <input type=number> localization faster. r=smaug,masayuki
Status: NEW → ASSIGNED

It is always called from TextControlState, and always ends up in
TextControlState::ValueEquals, so we can avoid some indirection and just
use that.

Depends on D183282

This avoids a lot of useless time on things like GetValueAsDecimal(),
where we get the default value as a string, parse it as a number for
sanitization reasons, localize it to a string, just to parse it back to
a number.

Depends on D183283

Flags: needinfo?(emilio)

ni? to write the perf reftest.

Flags: needinfo?(mstange.moz)
Attached file perf reftest patch (untested) (deleted) —
Flags: needinfo?(mstange.moz)

Err sorry that ni? Was meant to be for me!

But thanks I'll test it :)

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6231ae7b201 Remove dead nsTextControlFrame::GetText. r=masayuki
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b48a8a347379 Remove always-true aIgnoreWrap parameter from TextControlElement::GetTextEditorValue. r=masayuki
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2a2ee1032045 Remove nsTextControlFrame::TextEquals. r=masayuki

Intentionally not in the PGO list.

Flags: needinfo?(emilio)
Severity: -- → S3
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f457492531e8 Make <input type=number> localization faster. r=masayuki
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7799c0e4b59a Don't localize <input type=number>'s default value unless for display. r=masayuki
Keywords: regression
Regressed by: 981248

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

status-firefox115: --- → affected
status-firefox116: --- → affected
status-firefox-esr102: --- → affected
status-firefox-esr115: --- → affected

Reopening for the last two patches.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

https://hg.mozilla.org/mozilla-central/rev/f457492531e8
https://hg.mozilla.org/mozilla-central/rev/7799c0e4b59a

Status: REOPENED → RESOLVED
Closed: 1 year ago → 1 year ago
Resolution: --- → FIXED

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

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

For more information, please visit BugBot documentation.

Flags: needinfo?(emilio)

Not quite risk-free.

status-firefox116: affected → wontfix
Flags: needinfo?(emilio)

With the latest nightly, we are as fast as Chrome now.
Chrome: 21ms
Nightly: 22-23ms (28ms with profiler active: https://share.firefox.dev/3XMhqTr)

Duplicate of this bug: 1784401
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bbb73c8e087 Add a perf reftest. r=perftest-reviewers,afinder
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: