967752 - tresize talos regression on January 28th team seen on fx-team

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P4)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

As seen on graph server:
http://graphs.mozilla.org/graph.html#tests=[[254,132,33]]&sel=none&displayrange=30&datatype=running

and on datazilla:
https://datazilla.mozilla.org/?start=1390643891&stop=1391310782&product=Firefox&repository=Fx-Team-Non-PGO&test=tresize&page=tresize&graph_search=30d6d1f88b00&x86_64=false&project=talos

This appears to happen on linux, windows and mac

here is the checkin that caused the regression:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=30d6d1f88b00

here is the code for the test:
http://hg.mozilla.org/build/talos/file/79702830f03d/talos/startup_test/tresize-test.html

and a description:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tresize

This regression is seen on all branches and retriggers of the talos chrome suite shows that this patch from bug 962844 is the root cause.

Comment 1

[Dão Gottwald [:dao]]

I suppose it's possible that the navigation toolbar now overflows later in the test due to the reduced min-with of the location bar, which might affect the result. I wouldn't worry about that, as it's too dependent on the arbitrary numbers used in the test.

Or maybe there's a real regression due to the use of the "vw" CSS unit. That I think would be a Core/Layout bug.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

who could we query from core/layout to look into this?  Should we modify the test at all?

Comment 3

[Dão Gottwald [:dao]]

What's the easiest to run that test locally? I could start with trying to find out which of my guesses are correct (or whether there's some other strange thing going on).

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

many people have good luck following these instructions:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

I would use:
talos -e <firefox> -a tresize --develop --results_url resize.out --datazilla-url resize.json

since this is a large change, comparing the output of resize.out between two different versions of <firefox> should be easy to spot.

Comment 5

[Dão Gottwald [:dao]]

I'll try this tomorrow.

Comment 6

[Dão Gottwald [:dao]]

I ended up using the try server to test my hypotheses. This appears to undo the regression:

https://hg.mozilla.org/try/rev/3dcb5f29b0bf

So it does look like the vw unit is pretty expensive...

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :dao!

Comment 8

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

Yes, use of viewport units is expensive since it requires us to recompute style when the viewport resizes.  While there are things we could do to improve things, and chip away at the problem gradually, I'm not sure it's worth it.  I think it's easier to optimize for responsive layout that's part of the layout system (e.g., flexbox, percentage sizes).

I don't think the bug on the regression belongs in Core; this is a feature that's inherently slower than what you're using before.

Comment 9

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

Should this bug move elsewhere?

Comment 10

[Dão Gottwald [:dao]]

I suppose we could avoid the perf problem by replacing this:

#identity-icon-labels {
  max-width: 12vw;
}

with this:

#identity-icon-labels {
  max-width: 18em;
}
@media (max-width: 700px) {
  #identity-icon-labels {
    max-width: 70px;
  }
}
@media (max-width: 600px) {
  #identity-icon-labels {
    max-width: 60px;
  }
}
@media (max-width: 500px) {
  #identity-icon-labels {
    max-width: 50px;
  }
}

This feels like a hack, though. Then again, using vw was a hack in the first place. Ideally, we want #identity-icon-labels' max-width to be relative to its container's current width.

Comment 11

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

Does max-width: 12% (or whatever the right number is) not work?

Comment 12

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

Also, it might make sense to have a bug on making vw faster, but it shouldn't be this bug.

Comment 13

[Dão Gottwald [:dao]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #11)
> Does max-width: 12% (or whatever the right number is) not work?

That's what I tried first. Doesn't appear to have any effect.

Comment 14

[Dão Gottwald [:dao]]

Ok, I got it to work (by trying random stuff without fully understanding how XUL layout works here).

Comment 15

[Dão Gottwald [:dao]]

I was wrong, I didn't get the relative max-width to work. What I saw was 12vw still taking effect...

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

resorting to the workaround from comment 10

Comment 17

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/fb11c33c37ce

Comment 18

[Carsten Book [:Tomcat]]

had to back this out for mochitest-bc issues like https://tbpl.mozilla.org/php/getParsedLog.php?id=35213366&tree=Fx-Team windows only somehow

Comment 19

[Dão Gottwald [:dao]]

Attachment: same patch with browser_windowopen_reflows.js updated

https://hg.mozilla.org/integration/fx-team/rev/d3dd571c030a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3dd571c030a

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8381971 [details] [diff] [review]
same patch with browser_windowopen_reflows.js updated

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 962844
User impact if declined: pretty significant tresize regression, but unclear to what extent that's visible to users
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d90aa42194e

Comment 23

[Ioana (away)]

(In reply to Dão Gottwald [:dao] (on vacation from March 21 to 28) from comment #21)
> ... unclear to what extent that's visible to users

I'm not so sure what the verifyme was added for, given the above and the fact that this is covered automatically. Please let me know if there is anything specific that you want QA for here.

Comment 24

[Tracy Walker [:tracy]]

Joel, from datazilla, the regression in early Feb has obviously been reduced. Can you verify that is true for both Fx29 and Fx30?

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

I have verified this is not an issue on beta (29) and aurora (30).  On inbound there is a slight issue, but it is really small, probably related to something else than the Jan 28th regression.