Closed Bug 1351924 Opened 3 years ago Closed 11 months ago

Use nsChangeHint_UpdateComputedBSize to properly set the IsBResize() flag on reflow input

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox55 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox70 --- fixed

People

(Reporter: bzbarsky, Assigned: dbaron)

References

(Blocks 4 open bugs, Regressed 1 open bug)

Details

(Keywords: perf, Whiteboard: [qf:p2:responsiveness][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?
Flags: needinfo?(bugs)
Blocks: 1348848
Blocks: FastReflows
Flags: needinfo?(bugs)
Assignee: nobody → dholbert
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.
Flags: needinfo?(dholbert)
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.
Whiteboard: [qf:p1] → [qf:p2]
Blocks: 1403656
Keywords: perf
Whiteboard: [qf:p2] → [qf:p1]
Whiteboard: [qf:p1] → [qf:i60][qf:p1]
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
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?
Assignee: dholbert → nobody
Flags: needinfo?(dholbert) → needinfo?(mreavy)
(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!
Flags: needinfo?(mreavy) → needinfo?(cam)
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.
Flags: needinfo?(cam)
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.
Priority: -- → P2
Well, asking does not hurt: I'm new, is this feasible for me? Can anybody mentor?
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.
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.
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.
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]
potentially contributes to crashes according to bug 1403656, so => major
Severity: normal → major
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
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.
Assignee: cpearce → nobody
Status: ASSIGNED → NEW
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
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.
Assignee: cpearce → nobody
Status: ASSIGNED → NEW
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.
Fixing this may also fix bug 1403656.  Cam, can you take a look or help find someone to investigate?
Flags: needinfo?(cam)
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.
Whiteboard: [qf:p1:f64] → [qf:p2:responsiveness]

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.

Assignee: nobody → dholbert
Assignee: dholbert → nobody
Attachment #9002997 - Attachment is obsolete: true
Whiteboard: [qf:p2:responsiveness] → [qf:p2:responsiveness][layout:triage-discuss]

Going to get this on our backlog for Q3.

Whiteboard: [qf:p2:responsiveness][layout:triage-discuss] → [qf:p2:responsiveness]
Whiteboard: [qf:p2:responsiveness] → [qf:p2:responsiveness][layout-sydney-2018]

I merged this to tip again, made a bunch of revisions, and pushed to try to see what things look like.

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.

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.

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.

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.

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.

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

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: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #9077176 - Attachment description: Bug 1351924 - Add reftests for correctness bug fixed by this bug. r?dholbert → Bug 1351924 - Add reftests for correctness bug fixed by this bug.
Attachment #9077177 - Attachment description: Bug 1351924 - 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 → Bug 1351924 - 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.
No longer blocks: 1403656

Here's a new try run rebased on top of the dependent bugs.

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

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.

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

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Duplicate of this bug: 1576864
Regressions: 1584018

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.

Regressions: 1605868
You need to log in before you can comment on or make changes to this bug.