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

VERIFIED FIXED in Firefox 29

Status

()

defect
P4
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jmaher, Assigned: dao)

Tracking

(Blocks 1 bug, {perf, regression})

Trunk
Firefox 30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [talos_regression][Australis:P3])

Attachments

(1 attachment, 1 obsolete attachment)

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.
who could we query from core/layout to look into this?  Should we modify the test at all?
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).
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.
I'll try this tomorrow.
Flags: needinfo?(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...
Flags: needinfo?(dao)
thanks :dao!
Component: Location Bar → CSS Parsing and Computation
Product: Firefox → Core
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.
Priority: -- → P4
Should this bug move elsewhere?
Flags: needinfo?(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.
Flags: needinfo?(dao)
Does max-width: 12% (or whatever the right number is) not work?
Also, it might make sense to have a bug on making vw faster, but it shouldn't be this bug.
Component: CSS Parsing and Computation → Location Bar
Product: Core → Firefox
(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.
Ok, I got it to work (by trying random stuff without fully understanding how XUL layout works here).
Assignee: nobody → dao
I was wrong, I didn't get the relative max-width to work. What I saw was 12vw still taking effect...
Posted patch patch (obsolete) — Splinter Review
resorting to the workaround from comment 10
Attachment #8379856 - Flags: review?(jaws)
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
Whiteboard: [talos_regression] → [talos_regression][Australis:P3]
https://hg.mozilla.org/mozilla-central/rev/d3dd571c030a
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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
Attachment #8381971 - Flags: approval-mozilla-aurora?
Attachment #8381971 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
(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.
Flags: needinfo?(twalker)
Joel, from datazilla, the regression in early Feb has obviously been reduced. Can you verify that is true for both Fx29 and Fx30?
Flags: needinfo?(twalker) → needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.