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?
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.
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]
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.
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.
potentially contributes to crashes according to bug 1403656, so => major
Severity: normal → major
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
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.
You need to log in before you can comment on or make changes to this bug.