Shrink-to-fit InlineFormattingContext
Created attachment 391941 [details] Patch
Created attachment 391942 [details] Patch
I've inserted logging of actually used capacity, and run several web pages including Gmail, webkit.org, Speedometer. The result is large part of this is only 1. I think our inlineCapacity for Display::InlineContent is too large. Data collected from Speedometer2. RUNS 1 => 19257 2 => 4 3 => 2 4 => 4 5 => 1 LINEBOXES 1 => 19257 2 => 5 3 => 3 4 => 2 5 => 1 I think we should pick Vector<..., 1>.
*** Bug 208350 has been marked as a duplicate of this bug. ***
I'm now collecting PLT5's data.
Speedometer has a very specific and uncommon text content structure and I don't think we should optimize for this uncommon case. Normally we generate one (or a few) runs per line and a common paragraph (think of it as a section on a news page) has many lines of text. On live nytimes.com it averages over 2.1 for me. Apple.com has around 1.6 runs. Book content has an average over 10 for sure. Also keep in mind the low number is also an indication of low coverage of this feature which will change drastically very soon, so the number of runs will increase as well.
This is PLT5 data. RUNS 1 => 6910 2 => 813 3 => 1306 4 => 546 5 => 175 6 => 145 7 => 43 8 => 5 9 => 39 17 => 16 LINEBOXES 1 => 6925 2 => 1517 3 => 587 4 => 546 5 => 175 6 => 145 7 => 43 8 => 5 9 => 39 17 => 16
CDF from PLT5's data. To me, picking 4 sounds safe, it covers 95%. RUNS 1 => 6910, CDF 0.6911382276455291% 2 => 813, CDF 0.7724544908981796% 3 => 1306, CDF 0.9030806161232247% 4 => 546, CDF 0.9576915383076615% 5 => 175, CDF 0.9751950390078016% 6 => 145, CDF 0.9896979395879176% 7 => 43, CDF 0.993998799759952% 8 => 5, CDF 0.994498899779956% 9 => 39, CDF 0.9983996799359872% 17 => 16, CDF 1.0% LINEBOXES 1 => 6925, CDF 0.6926385277055411% 2 => 1517, CDF 0.844368873774755% 3 => 587, CDF 0.9030806161232247% 4 => 546, CDF 0.9576915383076615% 5 => 175, CDF 0.9751950390078016% 6 => 145, CDF 0.9896979395879176% 7 => 43, CDF 0.993998799759952% 8 => 5, CDF 0.994498899779956% 9 => 39, CDF 0.9983996799359872% 17 => 16, CDF 1.0%
(In reply to Yusuke Suzuki from comment #8) > CDF from PLT5's data. To me, picking 4 sounds safe, it covers 95%. > > RUNS > 1 => 6910, CDF 0.6911382276455291% > 2 => 813, CDF 0.7724544908981796% > 3 => 1306, CDF 0.9030806161232247% > 4 => 546, CDF 0.9576915383076615% > 5 => 175, CDF 0.9751950390078016% > 6 => 145, CDF 0.9896979395879176% > 7 => 43, CDF 0.993998799759952% > 8 => 5, CDF 0.994498899779956% > 9 => 39, CDF 0.9983996799359872% > 17 => 16, CDF 1.0% > LINEBOXES > 1 => 6925, CDF 0.6926385277055411% > 2 => 1517, CDF 0.844368873774755% > 3 => 587, CDF 0.9030806161232247% > 4 => 546, CDF 0.9576915383076615% > 5 => 175, CDF 0.9751950390078016% > 6 => 145, CDF 0.9896979395879176% > 7 => 43, CDF 0.993998799759952% > 8 => 5, CDF 0.994498899779956% > 9 => 39, CDF 0.9983996799359872% > 17 => 16, CDF 1.0% I am fine with 4 for not but please keep in mind we will increase this number over time.
Created attachment 391951 [details] Patch
(In reply to zalan from comment #9) > (In reply to Yusuke Suzuki from comment #8) > > CDF from PLT5's data. To me, picking 4 sounds safe, it covers 95%. > > > > RUNS > > 1 => 6910, CDF 0.6911382276455291% > > 2 => 813, CDF 0.7724544908981796% > > 3 => 1306, CDF 0.9030806161232247% > > 4 => 546, CDF 0.9576915383076615% > > 5 => 175, CDF 0.9751950390078016% > > 6 => 145, CDF 0.9896979395879176% > > 7 => 43, CDF 0.993998799759952% > > 8 => 5, CDF 0.994498899779956% > > 9 => 39, CDF 0.9983996799359872% > > 17 => 16, CDF 1.0% > > LINEBOXES > > 1 => 6925, CDF 0.6926385277055411% > > 2 => 1517, CDF 0.844368873774755% > > 3 => 587, CDF 0.9030806161232247% > > 4 => 546, CDF 0.9576915383076615% > > 5 => 175, CDF 0.9751950390078016% > > 6 => 145, CDF 0.9896979395879176% > > 7 => 43, CDF 0.993998799759952% > > 8 => 5, CDF 0.994498899779956% > > 9 => 39, CDF 0.9983996799359872% > > 17 => 16, CDF 1.0% > > I am fine with 4 for not but please keep in mind we will increase this > number over time. Right. For now, 4 looks a balanced number.
Comment on attachment 391951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391951&action=review > Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:136 > + formattingState().shrinkToFit(); Another design choice is calling this from low-memory-warning, but this sounds like a hack
Another idea is adjusting this number at runtime. We can do that for growth factor etc. But here, we are changing initial-capacity to avoid any growth etc. And this is compile time number (because we do not want to allocate memory three times for each allocation of Display::InlineContent). So, we are now collecting the data AOT.
(In reply to Yusuke Suzuki from comment #13) > Another idea is adjusting this number at runtime. We can do that for growth > factor etc. But here, we are changing initial-capacity to avoid any growth > etc. And this is compile time number (because we do not want to allocate > memory three times for each allocation of Display::InlineContent). So, we > are now collecting the data AOT. I think having this constant value is okay for now. The patch however needs some work because: 1. shrinkToFit is an actual layout concept and calling this function like that is extremely confusing. 2. there's a better place for this call (outside of core layout) I can re-work this patch if you don't mind.
(In reply to zalan from comment #14) > (In reply to Yusuke Suzuki from comment #13) > > Another idea is adjusting this number at runtime. We can do that for growth > > factor etc. But here, we are changing initial-capacity to avoid any growth > > etc. And this is compile time number (because we do not want to allocate > > memory three times for each allocation of Display::InlineContent). So, we > > are now collecting the data AOT. > > I think having this constant value is okay for now. The patch however needs > some work because: > 1. shrinkToFit is an actual layout concept and calling this function like > that is extremely confusing. > 2. there's a better place for this call (outside of core layout) > I can re-work this patch if you don't mind. You can do that :) Thanks
<rdar://problem/59875758>
Created attachment 391959 [details] Patch by Yusuke Suzuki
Created attachment 391960 [details] Patch by Yusuke
Comment on attachment 391960 [details] Patch by Yusuke View in context: https://bugs.webkit.org/attachment.cgi?id=391960&action=review > Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp:114 > + m_inlineFormattingState.shrinkDisplayInlineContent(); Couldn’t this be done in generic IFC code?
(In reply to Antti Koivisto from comment #19) > Comment on attachment 391960 [details] > Patch by Yusuke > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391960&action=review > > > Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp:114 > > + m_inlineFormattingState.shrinkDisplayInlineContent(); > > Couldn’t this be done in generic IFC code? We need to come up with some strategy for shrinking formatting state data in general (for all the other formatting contexts) until then I think the integration layer is as good as any.
Comment on attachment 391960 [details] Patch by Yusuke Clearing flags on attachment: 391960 Committed r257637: <https://trac.webkit.org/changeset/257637>
All reviewed patches have been landed. Closing bug.
Thanks Alan and Antti!