1344377 - Jank with unnecessary frame reconstruction & layout, when tweaking "position" on root of deeply-nested frame tree

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load attached testcase.
 2. Click "FIRST" (and then wait a few seconds for page to update)
 2. Click "SECOND" (and then wait for alert to appear)

ACTUAL RESULTS:
Alert reports times of 4+ seconds (4000ms).

EXPECTED RESULTS:
Shorter times. e.g. chrome gives 9ms


NOTES w.r.t. other bugs:
 - I'm spinning this off of bug 1342220 (the testcase is roughly based on the Twitter scenario in that bug)
 - This might be the same as bug 1261484, under the hood.  We can dupe later, as-needed, but for now I'm filing this separately, because bug 1261484 is theoretical whereas here we've got an actual testcase with measurable performance.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simpler)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simpler)

Comment 4

[Daniel Holbert [:dholbert]]

Average on Windows with "testcase 2 (simpler)", attachment 8843488 [details] (rough average after performing a few measurements):

Measurement   Browser Version
===========   ================
 2000ms       Firefox Nightly 54 (2017-03-03)
 300ms        Edge 14
 3ms          Chrome 58

Comment 5

[Marco Zehe (:MarcoZ)]

CC'ing myself and Surkov, too, since this also shows our accessibility performance problem. In testcase 1, before the Alert appears after those about 2800 milliseconds, there are 20 or so seconds when accessibility is on that aren't measured with this, but which clearly come from us recreating the whole accessible stuff. And that's with E10S off, which in accessibility land is more performant still. Alex, please take a look at these and use them to measure where we spend all that time.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Daniel Holbert [:dholbert] from comment #0)
>  - This might be the same as bug 1261484, under the hood.  We can dupe
> later, as-needed, but for now I'm filing this separately, because bug
> 1261484 is theoretical whereas here we've got an actual testcase with
> measurable performance.

Also see bug 1261484 comment 7, which might deserve being split off as a separate bug.

Comment 7

[Daniel Holbert [:dholbert]]

This was [qf:p1] in the interests of improving a really bad case on twitter (Bug 1342220), but I'm going to try to address Bug 1342220 by suggesting that Twitter change their CSS, as noted in Bug 1342220 comment 21.

So this bug can now be [qf:p3] -- known perf issue & opportunity for optimization, but not known to impact websites (beyond twitter which we'll hopefully be addressing with outreach such that this won't be an issue for them).

Comment 8

[Mayank Bansal]

Testcase1 :
Nightly : 75ms (https://share.firefox.dev/4dySKoq)
Chrome: 5ms

Testcase2:
Nightly: 30ms (https://share.firefox.dev/46VNfgU)
Chrome: 2ms

Should this bug be kept open?

Comment 9

[Daniel Holbert [:dholbert]]

Looks like things are definitely much better; 75ms / 30ms are much better than ~4000ms.

Might be good to add a perf-reftest here (leaving needinfo open as a reminder); but yeah, I don't think it's worth worrying about the remaining performance difference (for this intentionally-pathological content) too much, in the absence of known real-world site impact.