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

NEW
Unassigned

Status

()

P2
major
2 years ago
a month ago

People

(Reporter: bzbarsky, Unassigned)

Tracking

(Blocks: 5 bugs, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [qf:p1:f64])

Attachments

(2 attachments)

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)
(Reporter)

Updated

2 years ago
Blocks: 1348848
Blocks: 1341750
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]
(Reporter)

Updated

a year ago
Blocks: 1403656

Updated

10 months ago
Whiteboard: [qf:p2] → [qf:p1]

Updated

10 months ago
Whiteboard: [qf:p1] → [qf:i60][qf:p1]

Updated

10 months ago
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]

Updated

7 months ago
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

Comment 7

5 months ago
Well, asking does not hurt: I'm new, is this feasible for me? Can anybody mentor?
(Reporter)

Comment 8

5 months 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.
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

5 months 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

4 months ago
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]

Comment 11

4 months ago
potentially contributes to crashes according to bug 1403656, so => major
Severity: normal → major

Updated

4 months ago
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]

Updated

3 months ago
Blocks: 1471314

Updated

2 months ago
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
Created attachment 9002997 [details] [diff] [review]
Work in progress patch; fixes correctness issue in testcase, not perf issue
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Created attachment 9003001 [details]
Testcase for correctness issue

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
You need to log in before you can comment on or make changes to this bug.