(CANCELED) Limit word length to shape when !getTypesettingFeatures

Limit word length to shape when !getTypesettingFeatures

BUG=603398

[kojii, 2016-04-17 08:12:13]

Patchset #3 (id:40001) has been deleted

[kojii, 2016-04-17 08:47:37]

Description was changed from

==========
hb-long

BUG=603398
==========

to

==========
Limit word length to shape when !typographicFeature

BUG=603398
==========

[kojii, 2016-04-17 08:47:53]

Description was changed from

==========
Limit word length to shape when !typographicFeature

BUG=603398
==========

to

==========
Limit word length to shape when !getTypesettingFeatures

BUG=603398
==========

[kojii, 2016-04-17 10:22:29]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2016-04-17 10:22:30]

WDYT?

I wasn't aware we have kerning/ligatures turned on even when
!getTypesettingFeatures(), so this CL breaks such kerning/ligatures for words
longer than 15 chars. Failing tests indicate where they are.

We could limit this to fixed-pitch (for 8bit range) fonts, but we don't have a
flag for it today, we need to add it if we were going this route.

We could check kerning/ligature table for 8bit range and enable only when it's
safe.

Or, given we're looking forward to the large refactoring, I can investigate how
to fix break-all perf issue without changing break-word. Before the CL,
break-word was per-charcter, not per-prefix. Only break-all was per-perfix, so
the fix for break-word wasn't critical. Given this regression, I'm starting to
feel that putting non-critical fix into the CL was a wrong decision.

I'm currently leaning towards to the last option, but your thoughts appreciated.

[kojii, 2016-04-17 10:28:12]

Two more options I came up with:
C. Take ideas from your design, give available width to shaper to reduce the
length of shaping.
D. Help you to finish the refactoring during M52 and merge it back to M51. Most
forward-looking but probably too risky?

[eae, 2016-04-17 18:21:16]

If we limit it to something like 128 instead the kerning/ligature breaking is
mitigated and we retain most of the performance win.

[kojii, 2016-04-18 02:06:46]

On 2016/04/17 at 18:21:16, eae wrote:
> If we limit it to something like 128 instead the kerning/ligature breaking is
mitigated and we retain most of the performance win.

Unfortunately, in Official build (-O1 option IIUC), 128 didn't get any
(crbug.com/603398#c10). In debug, it mitigates by 5.5 times though (comment 7 of
the bug.)

Current: 1650
Limit=1024: 1600
Limit=128: 1650
Limit=16: 3050
Limit=15: 95

[drott, 2016-04-18 06:39:25]

I am aware this is a pressing issue, but if possible, I would prefer not too
depend on getTypeSettingFeatures() - I'd like to see this going away in the
future, or at least be changed to something like "requiresOpenTypeShaping" if
that would be more descriptive.

[kojii, 2016-04-18 06:43:59]

On 2016/04/18 at 06:39:25, drott wrote:
> I am aware this is a pressing issue, but if possible, I would prefer not too
depend on getTypeSettingFeatures() - I'd like to see this going away in the
future, or at least be changed to something like "requiresOpenTypeShaping" if
that would be more descriptive.

Can't agree more, all these should be gone when new line breaker is in. We just
need a fix that can merged back to M51 for (hopefully) a few weeks.

Can you have a look at https://codereview.chromium.org/1900513002 ? With the
feedback from both of you, I'm started to feel that it's better approach.

[eae, 2016-04-18 18:09:35]

On 2016/04/18 06:43:59, kojii wrote:
> On 2016/04/18 at 06:39:25, drott wrote:
> > I am aware this is a pressing issue, but if possible, I would prefer not too
> depend on getTypeSettingFeatures() - I'd like to see this going away in the
> future, or at least be changed to something like "requiresOpenTypeShaping" if
> that would be more descriptive.
> 
> Can't agree more, all these should be gone when new line breaker is in. We
just
> need a fix that can merged back to M51 for (hopefully) a few weeks.
> 
> Can you have a look at https://codereview.chromium.org/1900513002 ? With the
> feedback from both of you, I'm started to feel that it's better approach.

Agreed, I like that one better. If that is enough to offset the regression that
would be fantastic. If not we can revisit this.

I'm a bit worries that always breaking at 15 will saturate the cache really
quickly.