Performance-regression for eclipse RAP based applications (reflow caused by setting scrollLeft)

VERIFIED FIXED in Firefox 67

Status

()

defect
P2
normal
VERIFIED FIXED
16 days ago
6 days ago

People

(Reporter: linuxhippy, Assigned: emilio)

Tracking

(Regression, {perf, regression})

67 Branch
mozilla69
Points:
---

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox67+ verified, firefox67.0.5 wontfix, firefox68+ verified, firefox69+ verified)

Details

Attachments

(2 attachments)

Reporter

Description

16 days ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

  1. used an internal application based on the RAP framework

Actual results:

  1. noticed how navigating to a particular site got so much slower (12s -> 35s) when running an updated firefox site

Expected results:

  1. firefox should not regress for this case and be as fast as chrome (9s)
Reporter

Comment 1

16 days ago

some more information:

Unfortunately I wasn't successful in creating a standalone reproducing test-case (maybe I need to create more unrelated DOM stuff just to make the reflow more expensive).

Summary: Performance-regression for eclipse RAP based applications → Performance-regression for eclipse RAP based applications (reflow caused by setting scrollLeft)
Reporter

Comment 2

16 days ago

I finally succeeded in creating a stand-alone testcase.

A ready-to-run war-file, as well as a fully configured tomcat with the application pre-deployed are available at:
https://drive.google.com/drive/folders/1RANc_MHhuLf6uy4l9m7Nu7eoAFsSC8Ng?usp=sharing

Steps to start the tomcat:

  1. Un-7-zip
  2. bin/startup.sh (or the windows equivalent)

Open the URL: http://localhost:8080/bench/bench

a button with "perform slow operation" appears -> click it.

Results:
Chrome: ~6s
Firefox 69 Nightly: 50s

Reporter

Comment 3

16 days ago

Firefox 65: 3s

so Firefox 65 -> 67 caused an almost 20x slowdown for this particular test.

Reporter

Updated

15 days ago
Keywords: perf, regression
Reporter

Updated

15 days ago
Version: 69 Branch → 67 Branch
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Flags: needinfo?(emilio)
OS: Unspecified → Linux
Product: Firefox → Core
Regressed by: 1523071
Hardware: Unspecified → x86_64
Has Regression Range: --- → yes
Has STR: --- → yes

Huh, that regression range seems quite surprising... Will take a look though.

Assignee

Updated

15 days ago
Assignee: nobody → emilio

I'm not sure I'll be able to take a look before Monday because I need to get a plane today, but just wanted to confirm that I can reproduce the regression. Thanks so much for the effort in constructing a test-case.

Looks like a nasty perf regression. I want to understand what's going on since it doesn't quite make sense to me off-hand, patch that allegedly introduced it should be mostly performance-neutral.

Priority: -- → P2
Reporter

Comment 7

14 days ago

Hi Emilio, thanks a lot for looking into this - I was also puzzled, the code in the commits looks rather harmless.

(In reply to Clemens Eisserer from comment #7)

Hi Emilio, thanks a lot for looking into this - I was also puzzled, the code in the commits looks rather harmless.

Ugh, got it, silly me :(

That website heavily relies on one of our reflow optimizations that I accidentally broke. I'll fix and add a test.

Flags: needinfo?(emilio)

That website could be infinitely faster btw (both in Firefox and Chromium), if they avoided the pattern:

  • element.style.left = something that depends on offsetHeight or other layout-dependent value

Comment on attachment 9069258 [details]
Bug 1555962 - Fix a typo that makes us reflow too much in abspos subtrees. r=jwatt

Beta/Release Uplift Approval Request

  • User impact if declined: Sizable perf regression.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Can use the test in comment 2, or the file added in https://bugzilla.mozilla.org/attachment.cgi?id=9069259.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Re-introduces an optimization which I broke in some cases.
  • String changes made/needed: none
Attachment #9069258 - Flags: approval-mozilla-release?
Attachment #9069258 - Flags: approval-mozilla-beta?
Assignee

Updated

14 days ago
Attachment #9069259 - Flags: approval-mozilla-release?
Attachment #9069259 - Flags: approval-mozilla-beta?
Assignee

Updated

14 days ago
Flags: qe-verify+

Tracking for 67 as this fix seems like a potential ride-along for a future dot release.

leave-open until I land the test too.

Keywords: leave-open

Comment 15

13 days ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84c35e9f9c74
Fix a typo that makes us reflow too much in abspos subtrees. r=jwatt

Comment 16

13 days ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5cbe8daec8b
Add a perf test which is an order of magnitude slower without the previous patch. r=heycam
Assignee

Updated

12 days ago
Status: ASSIGNED → RESOLVED
Closed: 12 days ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla69

Looks fixed, starting in Mozilla Firefox Nightly 69.0a1 (2019-06-04), so I'm marking this bug as VERIFIED.
Thank you very much! \o/

@ Clemens Eisserer - As reporter, could you confirm this please?

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Reporter

Comment 19

11 days ago

just tested against the production code - everything is as fast as before :)
Thanks a lot!

Flags: needinfo?(linuxhippy)

Comment on attachment 9069258 [details]
Bug 1555962 - Fix a typo that makes us reflow too much in abspos subtrees. r=jwatt

fix a logic error, approved for 68.0b8

Attachment #9069258 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9069259 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I've reproduced this issue on Fx 69.0a1 - 2019-05-31 (Ubuntu 18.04 x64) and I can confirm also, it is fixed with the latest Nightly 69.0a1 - 2019-06-04. I'm letting the qa+ flag on, in order to verify this issue once the fix will land in Fx 68.0b8.

(In reply to Clemens Eisserer from comment #19)

just tested against the production code - everything is as fast as before :)
Thanks a lot!

You're welcome, thanks so much for reporting it :)

I can confirm the issue is fixed on Beta also (68.0b8 - 20190606101422) with Ubuntu 18.04 x64.

Flags: qe-verify+

Comment on attachment 9069259 [details]
Bug 1555962 - Add a perf test which is an order of magnitude slower without the previous patch. r=heycam

Fix for a logic error covered by tests and uplift verified in 68b8, approcved for 67.0.2 thanks.

Attachment #9069259 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9069258 - Flags: approval-mozilla-release? → approval-mozilla-release+

Comment 27

8 days ago

"Platform: x86_64 Linux"
really?

Adding QE+ for 67.0.2.

Flags: qe-verify+

This issue is verified fixed with 67.0.2 (20190607204818) on Ubuntu 18.04 x64.

Flags: qe-verify+

Comment 30

6 days ago

(In reply to j.j. from comment #27)

"Platform: x86_64 Linux"
really?

not really, Platform -> All (this bug is obviously on Windows as well)

OS: Linux → All
Hardware: x86_64 → All
You need to log in before you can comment on or make changes to this bug.