Use nsChangeHint_UpdateComputedBSize to properly set the IsBResize() flag on reflow input
Categories
(Core :: Layout, defect, P2)
Tracking
()
People
(Reporter: bzbarsky, Assigned: dbaron)
References
(Blocks 4 open bugs, Regressed 1 open bug)
Details
(Keywords: perf, perf:responsiveness, Whiteboard: [layout-sydney-2018])
Attachments
(4 files, 1 obsolete file)
Right now we are kind of guessing in ReflowInput::InitResizeFlags whether our computed "auto" value means we actually have a block-direction resize or not. For that matter, we're guessing whether our computed _non-auto_ value means we have a block-direction resize or not. The right answer, per discussion with dbaron today, is most likely along the lines of "we have a block-direction resize if we had a style change with the nsChangeHint_UpdateComputedBSize hint" or so. The question is how to feed that information into here. My gut feeling is that fixing this could fix various cases (e.g. bug 1348848) where we're reflowing too much right now. Jet, do you know who might have time to look into this?
Updated•7 years ago
|
Updated•7 years ago
|
Comment 1•7 years ago
|
||
I'm intermittently away for the next few weeks, but I'm around next week & am hoping to get a chance to investigate this a bit then.
Comment 2•7 years ago
|
||
Sorry that I haven't gotten much substantial done here yet -- I'm bumping this to qf:p2 since I'm not convinced this needs to block 57. I may still circle back to this for 57, though.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Unassigning to reflect reality, as I haven't gotten to this yet and realistically won't get to it soon (as I've changed teams, am prepping to manage some interns, and have code-reviews & high-priority flexbox perf work to catch up on, and I've got a vacation coming up in a couple weeks). I believe this is still ~high-priority, though, in part for perf reasons (e.g. bug 1348848 per bz's comment 0 here), and in part because Tyson says fuzzers are quite frequently tripping over a bug that seems to be a version of this: bug 1403656. Maire, could you find someone to work on this?
Comment 4•6 years ago
|
||
(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11) from comment #3) > Unassigning to reflect reality, as I haven't gotten to this yet and > realistically won't get to it soon (as I've changed teams, am prepping to > manage some interns, and have code-reviews & high-priority flexbox perf work > to catch up on, and I've got a vacation coming up in a couple weeks). > > I believe this is still ~high-priority, though, in part for perf reasons > (e.g. bug 1348848 per bz's comment 0 here), and in part because Tyson says > fuzzers are quite frequently tripping over a bug that seems to be a version > of this: bug 1403656. > > Maire, could you find someone to work on this? Sure, Daniel. Thanks for the needinfo. Cameron -- Can you give me a (rough) assessment of the level-of-effort here and whether this could be a good "first bug" for some of our senior platform engineers who are new to Layout? Also, this is marked as a P1 perf/quantum flow bug. Do you agree that's the right priority (relative to other perf work we could do)? Even if it's not, I'm inclined to prioritize this one relatively high because it's slowing down Tyson. I welcome your thoughts/feedback on all of this. Thanks!
Comment 5•6 years ago
|
||
Yes, I think we should prioritize this; bug 1348848 is a high profile, table-layout-heavy site, and this kind of table layout is not uncommon. For someone who's happy to dive into unfamiliar reflow code, it would be a good moderate difficulty first layout bug.
Comment 6•6 years ago
|
||
Moving to a P2 so it doesn't fall off our radar for prioritizing, but I'll likely create a whiteboard tag for bugs like this that we want to work on soon.
Comment 7•6 years ago
|
||
Well, asking does not hurt: I'm new, is this feasible for me? Can anybody mentor?
Reporter | ||
Comment 8•6 years ago
|
||
Are you happy to dive into unfamiliar, somewhat complicated code? Especially when no one is really 100% an expert on it? I can probably mentor. I suspect dholbert might be able to as well, once he's back from vacation.
Comment 9•6 years ago
|
||
I think we do also have some newly-transferred-over layout engineers who are looking for projects to work on -- this is probably better-suited to one of them, with moderate experience in adjacent areas of Firefox code. @Daniel de Almeida: you might consider looking at bug 1452829 (as-yet unassigned), if you're looking for a "first bug" to start on! That's an optimization that should be pretty straightforward.
Comment 10•6 years ago
|
||
daniel/boris: I wasn't sure whether 'good first layout bug' in this case meant good for beginners or good for seasoned people already working on other parts of firefox, so now I guess it's the latter! I'll look into bug 1452829 thanks for the input.
Updated•6 years ago
|
Comment 11•6 years ago
|
||
potentially contributes to crashes according to bug 1403656, so => major
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 12•6 years ago
|
||
I am moving onto WebRender, so I won't be able to complete this. I will upload my patch and testcase. With dbaron's help I managed to discover and fix a correctness issue, but that didn't help with performance here.
Comment 13•6 years ago
|
||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
This is the testcase dbaron proposed at the Sydney 2018 layout work week which identified a correctness bug. My patch fixed the correctness issue, but turned out to not fix the perf issue.
Updated•6 years ago
|
Comment 15•6 years ago
|
||
This came up again in bug 1482938, which may be a duplicate. While it is too late to fix in 63, we could still take a patch for 65 nightly.
Comment 16•5 years ago
|
||
Fixing this may also fix bug 1403656. Cam, can you take a look or help find someone to investigate?
Comment 17•5 years ago
|
||
Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Here's the same WIP patch (attachment 9002997 [details] [diff] [review]), rebased to tip.
I tested to see if it helps with bug 1403656, but unfortunately it does not (the latest testcase there still crashes in a build with this rebased patch). Not entirely surprising, given comment 14's note about this not (yet) fixing perf issues.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Going to get this on our backlog for Q3.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
I merged this to tip again, made a bunch of revisions, and pushed to try to see what things look like.
Assignee | ||
Comment 21•5 years ago
|
||
And for comparison, here's a try run before my additional changes. If the differences between the runs are interesting, it might be worth separating out the change from ConvertsToPercentage()
to HasPercent()
(which I think is obviously correct) from the other changes.
Assignee | ||
Comment 22•5 years ago
|
||
Those two try runs had an identical set of failures: multicol-zero-height-001.xht
(two copies), layout/tables/reftests/1031934.html
, and layout/reftests/bugs/1263845.html
.
Assignee | ||
Comment 23•5 years ago
|
||
The failure of layout/tables/reftests/1031934.html
is, I think, a separate incremental reflow bug that needs to be fixed. The failure (a) goes away if I add html { overflow-y: scroll }
to suppress the horizontal resize that happens when we initially try reflowing with a scrollbar and (b) happens when (with the patch only) I resize the width of the page. I suspect there's something in the handling of visibility:collapse
that assumes that the entire table has been reflowed and goes wrong if it hasn't been.
Assignee | ||
Comment 24•5 years ago
|
||
At this point I've nearly fixed all of the test failures (2 of the 3 as separate bugs, one of which I still need to write the patch for but I'm pretty sure I know what will work; 1 as a revision to the patch here). There's still the issue that this doesn't fix one of the performance problems it was expected to fix. It does, however, fix some correctness issues and I might try to get it landed once I clean up the patch and write the necessary tests.
Assignee | ||
Comment 25•5 years ago
|
||
On top of the fixes for bug 1564308 and bug 1564649, I believe this will now pass all tests.
Although I don't think (though I should double-check with the latest revisions, but I think it's unlikely to have changed) this fixes the bug (bug 1348848) that it was intended to fix, it does:
- fix known correctness bugs
- optimize cases to reflow less where we were previously reflowing more than needed (as shown by the fact that those optimizations uncovered two existing bugs shown in our existing tests -- the ones listed in the previous paragraph -- where I could modify the tests so they showed the bugs today)
Thus I think it's worth landing this as it is so far, and then digging in to bug 1348848 as a followup.
Assignee | ||
Comment 26•5 years ago
|
||
Co-authored-by: L. David Baron <dbaron@dbaron.org>
Co-authored-by: Chris Pearce <cpearce@mozilla.com>
Assignee | ||
Comment 27•5 years ago
|
||
This reduces a bit of code complexity, fixes bugs where we weren't
reflowing enough, and optimizes additional cases that we couldn't
optimize in the past.
Co-authored-by: Chris Pearce <cpearce@mozilla.com>
Co-authored-by: L. David Baron <dbaron@dbaron.org>
Depends on D37609
Assignee | ||
Comment 28•5 years ago
|
||
Oh, and the test fix that was part of this bug rather than a separate bug was the fix for layout/reftests/bugs/1263845.html
which I ended up fixing by moving the clearing of the flag to nsFrame::DidReflow
. For a description of that test fix, see the comments in the main patch in both ReflowInput::InitResizeFlags
(referencing nsBlockReflowContext::ComputeCollapsedBStartMargin
, which was the problem in that case) and nsFrame::DidReflow
.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 29•5 years ago
|
||
Here's a new try run rebased on top of the dependent bugs.
Comment 30•5 years ago
|
||
Pushed by dbaron@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/27bcd63fde99 Add reftests for correctness bug fixed by this bug. r=dholbert https://hg.mozilla.org/integration/autoland/rev/05f675735d39 Keep separate flags for whether block-size has changed and whether percentages derived from the block-size have changed, and make better decisions about what needs reflow. r=dholbert
Assignee | ||
Comment 31•5 years ago
|
||
Also, for the record, I should note that I believe that on 32-bit systems this patch increased the size of nsIFrame
by a word. If I'm looking at things correctly, it shouldn't have changed anything on 64-bit systems, though. I'm not aware of spare bits we have elsewhere, though, and I've been saying for a while that we shouldn't be overly conservative about adding 1-bit member variables to nsIFrame
despite that at some point (now) it will increase the size of frames by a word.
Assignee | ||
Comment 32•5 years ago
|
||
Oops, I think it increased the size by a word on both 32-bit and 64-bit. (I misread mOverflow
as a struct
when it's actually a union
.)
Comment 33•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/27bcd63fde99
https://hg.mozilla.org/mozilla-central/rev/05f675735d39
Updated•5 years ago
|
Assignee | ||
Comment 35•5 years ago
|
||
I filed bug 1586175 to back this bug out for Firefox 70 (only) in order to give us more time to fix regression bug 1584018.
Updated•2 years ago
|
Description
•