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

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Clemens Eisserer]

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:

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

Expected results:

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

Comment 1

[Clemens Eisserer]

some more information:

• mozregression led me to the following push range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d6a02521683879d28e94e2912c09fcec1645d2f3&tochange=32b792a2a7a4c1876a2de67e752881ebcb3e08a3

• a firefox profile is available here: http://bit.ly/2WlMInL
  The issue seems to be caused by setting Element.scrollLeft to some value (the framework does this for each and every element), almost all the time is spent there.

• the code in question is at: https://github.com/eclipse/rap/blob/master/bundles/org.eclipse.rap.rwt/js/rwt/widgets/base/Widget.js
  disableScrolling -> _blockScrollingOnAppear

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).

Comment 2

[Clemens Eisserer]

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

Comment 3

[Clemens Eisserer]

Firefox 65: 3s

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Emilio Cobos Álvarez (: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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 7

[Clemens Eisserer]

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1555962 - Fix a typo that makes us reflow too much in abspos subtrees. r=jwatt

Ugh, I accidentally introduced this in bug 1523071. :(

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1555962 - Add a perf test which is an order of magnitude slower without the previous patch. r=heycam

Depends on D33434

Comment 11

[Emilio Cobos Álvarez (: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 12

[Emilio Cobos Álvarez (:emilio)]

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

Comment 13

[Pascal Chevrel:pascalc]

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

leave-open until I land the test too.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84c35e9f9c74
Fix a typo that makes us reflow too much in abspos subtrees. r=jwatt

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/84c35e9f9c74
https://hg.mozilla.org/mozilla-central/rev/d5cbe8daec8b

Comment 18

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

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?

Comment 19

[Clemens Eisserer]

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

Comment 20

[Julien Cristau [:jcristau]]

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

Comment 21

[Anca Soncutean, Desktop QA]

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.

Comment 22

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/00203ea1541d
https://hg.mozilla.org/releases/mozilla-beta/rev/b3edc708cfc7

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(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 :)

Comment 24

[Anca Soncutean, Desktop QA]

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

Comment 25

[Pascal Chevrel:pascalc]

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.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/4bde450ccf8a
https://hg.mozilla.org/releases/mozilla-release/rev/f0900e70cbef

Comment 27

[j.j.]

"Platform: x86_64 Linux"
really?

Comment 28

[Cristian Fogel [:cfogel]]

Adding QE+ for 67.0.2.

Comment 29

[Anca Soncutean, Desktop QA]

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

Comment 30

[j.j.]

(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)