Bug 208343 – Shrink-to-fit InlineFormattingContext

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yusuke Suzuki]

Shrink-to-fit InlineFormattingContext

[Yusuke Suzuki]

Created attachment 391941 [details]
Patch

[Yusuke Suzuki]

Created attachment 391942 [details]
Patch

[Yusuke Suzuki]

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

[Yusuke Suzuki]

*** Bug 208350 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

I'm now collecting PLT5's data.

[zalan]

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.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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%

[zalan]

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

[Yusuke Suzuki]

Created attachment 391951 [details]
Patch

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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.

[zalan]

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

[Yusuke Suzuki]

(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

[Radar WebKit Bug Importer]

<rdar://problem/59875758>

[zalan]

Created attachment 391959 [details]
Patch by Yusuke Suzuki

[zalan]

Created attachment 391960 [details]
Patch by Yusuke

[Antti Koivisto]

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?

[zalan]

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

[WebKit Commit Bot]

Comment on attachment 391960 [details]
Patch by Yusuke

Clearing flags on attachment: 391960

Committed r257637: <https://trac.webkit.org/changeset/257637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Thanks Alan and Antti!