Bug 99393 – Add support for 8 bit TextRuns for ports using HarfBuzz or HarfBuzz NG

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

See https://bugs.webkit.org/show_bug.cgi?id=97378 for reference.

[Levi Weintraub]

Created attachment 168822 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 168822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168822&action=review

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.h:60
> +        return stringFor8BitRun.characters16();

m_stringFor8BitRuns.characters16()? If so, should we make sure that get16BitCharactersFor8BitRun() called only once?

[Tony Chang]

Comment on attachment 168822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168822&action=review

> Source/WebCore/ChangeLog:9
> +        Adding support for 8 bit TextRuns for platforms using HarfBuzz and HarfBuzz NG.
> +        This involves converting 8 bit TextRuns to 16 bit in the complex text case, and

Do you have some perf or memory usage numbers?

[Levi Weintraub]

(In reply to comment #3)
> Do you have some perf or memory usage numbers?

I'm afraid not. Perhaps Michael Saboff does for the Mac port? Otherwise, if you have a suggestion on how to gather some good numbers I'd be happy to do it.

[Eric Seidel (no email)]

I might start by running run-perf-tests Layout/line-breaking.html, that does ascii-fast-path line breaking and might notice a speedup from text runs using only half the memory.

[Michael Saboff]

(In reply to comment #4)
> (In reply to comment #3)
> > Do you have some perf or memory usage numbers?
> 
> I'm afraid not. Perhaps Michael Saboff does for the Mac port? Otherwise, if you have a suggestion on how to gather some good numbers I'd be happy to do it.

I put performance numbers from running the PerformanceTests/Layout tests into comments for 96979,  See https://bugs.webkit.org/show_bug.cgi?id=96979#c9.

The final numbers were a little less than that, but basically follow the same trend.

[Benjamin Poulain]

Comment on attachment 168822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168822&action=review

Is there no version of those function taking a character iterator? This defeat the purpose of WTF_USE_8BIT_TEXTRUN :(

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.h:59
> +        m_stringsFor8BitRuns.append(stringFor8BitRun);

Is this your way of keeping the 16bits memory alive?
If it is, you should be passing a buffer to the caller instead.
If you have an other intent for m_stringsFor8BitRuns, what is it?

[Levi Weintraub]

(In reply to comment #7)
> (From update of attachment 168822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168822&action=review
> 
> Is there no version of those function taking a character iterator? This defeat the purpose of WTF_USE_8BIT_TEXTRUN :(

The purpose of using 8 bit text runs is to optimize the common cases where complex text uses 16 bit strings and simple text uses 8 bit. Unless I'm mistaken, the simple case should be properly handling 8 bit text runs without conversion and without change.

[Eric Seidel (no email)]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Do you have some perf or memory usage numbers?
> > 
> > I'm afraid not. Perhaps Michael Saboff does for the Mac port? Otherwise, if you have a suggestion on how to gather some good numbers I'd be happy to do it.
> 
> I put performance numbers from running the PerformanceTests/Layout tests into comments for 96979,  See https://bugs.webkit.org/show_bug.cgi?id=96979#c9.
> 
> The final numbers were a little less than that, but basically follow the same trend.

Yeah, I would expect line-layout to move, since we're now walking much smaller buffers in the fast-text-path.  Sadly Layout/ doesn't have any complex-text cases as far as I can tell.  I should talk to mitz about how to make some good ones.

[Eric Seidel (no email)]

Looks like my fancy new test in bug 100024 likes this change:

before:
median= 91.8549262304 runs/s, stdev= 1.37209469605 runs/s, min= 87.2826661652 runs/s, max= 92.2565152857 runs/s

after:
median= 94.9297505045 runs/s, stdev= 0.410723301989 runs/s, min= 94.27891947 runs/s, max= 95.7165641127 runs/s

I could re-run the before numbers to get more consistent results, but the "before" max seems outside of the "after" range anyway. :)

[Eric Seidel (no email)]

I tried testing this on a Mac, but it seems we don't actually use HarfBuzz on Mac.  I'll have to test this from my linux machine:
http://code.google.com/p/chromium/issues/detail?id=158620

[Eric Seidel (no email)]

Comment on attachment 168822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168822&action=review

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.h:74
> +    Vector<String> m_stringsFor8BitRuns;

I don't think this is necessary.  We should chat about this patch when your'e back.

[Levi Weintraub]

Created attachment 190149 [details]
Patch

[Levi Weintraub]

(In reply to comment #13)
> Created an attachment (id=190149) [details]
> Patch

The Vector was unnecessary, as we already normalized to a separate buffer.

[Eric Seidel (no email)]

Comment on attachment 190149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190149&action=review

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp:98
> +    String stringFor8BitRun;

So this lives long enough?  normalize doesnt' try to hang onto the bytes?

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:164
> +    String stringFor8BitRun;

So long as this lives long enough.

[Eric Seidel (no email)]

Comment on attachment 190149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190149&action=review

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.h:36
> +#include <wtf/text/WTFString.h>

Intentional?

[Levi Weintraub]

(In reply to comment #16)
> (From update of attachment 190149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190149&action=review
> 
> > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.h:36
> > +#include <wtf/text/WTFString.h>
> 
> Intentional?

Damn, no, that's vestigial. Fixing before landing.

[Levi Weintraub]

(In reply to comment #15)
> (From update of attachment 190149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190149&action=review
> 
> > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp:98
> > +    String stringFor8BitRun;
> 
> So this lives long enough?  normalize doesnt' try to hang onto the bytes?
> 
> > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:164
> > +    String stringFor8BitRun;
> 
> So long as this lives long enough.

It does, as it's copied into the normalized buffer inside this function.

[Levi Weintraub]

Created attachment 190160 [details]
Patch for landing

[Build Bot]

Comment on attachment 190160 [details]
Patch for landing

Attachment 190160 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16751624

New failing tests:
fast/xsl/xslt-missing-namespace-in-xslt.xml

[Levi Weintraub]

I'm going to wait for the cr-bots to process this, but the mac-wk2 failure is flake :-/

[WebKit Review Bot]

Comment on attachment 190160 [details]
Patch for landing

Clearing flags on attachment: 190160

Committed r144065: <http://trac.webkit.org/changeset/144065>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110896

[Stephen Chenney]

This broke Win7 and Win XP tests, in a rather serious manner. It was rolled out.

Links coming shortly.

[Stephen Chenney]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ftext%2Fcomplex-preferred-logical-widths.html%2Cfast%2Ftext%2Fword-space-with-kerning-2.html%2Cfast%2Ftext%2Ffont-kerning.html%2Cfast%2Ftext%2Ffont-variant-ligatures.html%2Cfast%2Ftext%2Fjustify-ideograph-leading-expansion.html%2Cfast%2Ftext%2Fword-space-with-kerning.html%2Cfast%2Ftext%2Fjustification-padding-mid-word.html
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css3%2Ffont-feature-settings-rendering.html%20fast%2Fcss%2Ftext-rendering.html%20fast%2Ftext%2Finternational%2Ftext-spliced-font.html%20fast%2Ftext%2Fshaping%2Fshaping-selection-rect.html%20fast%2Fwriting-mode%2Ftext-orientation-basic.html

[Eric Seidel (no email)]

Thanks Stephen

[Levi Weintraub]

Likewise, thanks Stephen and sorry for the noise. Sadly, I don't have a Windows machine to debug this... I can re-land for non-windows platforms or wait till someone can figure out what's going wrong there. The backtrace from the bots isn't too useful, though there are only a few places that assert !is8Bit()

crash log for DumpRenderTree (pid 2996):
STDOUT: <empty>
STDERR: ASSERTION FAILED: !is8Bit()
STDERR: Backtrace:
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65CC5559+9851929]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65EDA062+12033826]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65E7D3FA+11653818]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65CC5185+9850949]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A38C08+23956168]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A36BF8+23947960]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A3525B+23941403]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A2E973+23914547]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A2DFBF+23912063]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66A325D0+23930000]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66973B6D+23149101]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66972BEC+23145132]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66978FDB+23170715]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66978C92+23169874]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66973B83+23149123]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66972BEC+23145132]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66978FDB+23170715]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66978C92+23169874]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66973B83+23149123]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x66972BEC+23145132]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6687A6F2+22128050]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6687B563+22131747]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65F21A7C+12327228]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x659A8061+6586145]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65F43744+12465668]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65F43419+12464857]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65F430DE+12464030]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x659B1A69+6625577]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65CA09F7+9701559]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65C62FB1+9449073]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65BF3DE1+8993953]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65BF3EAC+8994156]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65BEFCC1+8977281]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65BF3F19+8994265]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65BF4226+8995046]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65F1D5A3+12309603]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x660859C7+13785223]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6642B2FE+17609150]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6642B51F+17609695]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6603B6E4+13481380]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6603B7F7+13481655]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x6656B9E5+18921637]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x667E071D+21497309]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x665D3A57+19347735]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x65EF53EE+12145326]
STDERR: 	webkit::ppapi::NPObjectToPPVar [0x61083CC9+545024]
STDERR: 	(No symbol) [0x0042177B]
STDERR: 	(No symbol) [0x0042B58C]
STDERR: 	(No symbol) [0x0042B1E1]
STDERR: 	(No symbol) [0x0042AC2C]
STDERR: 	base::win::OSInfo::processors [0x6426C91F+378667]
STDERR: 	base::win::OSInfo::processors [0x64278313+426271]
STDERR: 	base::win::OSInfo::processors [0x64278804+427536]
STDERR: 	base::win::OSInfo::processors [0x642796C6+431314]
STDERR: 	base::win::OSInfo::processors [0x64287FF4+491008]
STDERR: 	base::win::OSInfo::processors [0x64287102+487182]
STDERR: 	base::win::OSInfo::processors [0x642269EC+92152]
STDERR: 	base::win::OSInfo::processors [0x64277E59+425061]
STDERR: 	base::win::OSInfo::processors [0x64277BAE+424378]
STDERR: 	base::win::OSInfo::processors [0x64248DE9+232437]
STDERR: 	base::win::OSInfo::processors [0x64276E81+421005]
STDERR: 	(No symbol) [0x00364B6D]

[Levi Weintraub]

I'm going to try enabling this just for Linux & Mac, that way I can just file a bug for the Windows issue and try to find an owner for it.

[Levi Weintraub]

Created attachment 191067 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 191067 [details]
Patch

Fantastic.  YOu might add a comment to the gypi linking to the windows bug.

[Levi Weintraub]

(In reply to comment #31)
> (From update of attachment 191067 [details])
> Fantastic.  YOu might add a comment to the gypi linking to the windows bug.

Will do before landing. This'll make it easier for whoever is willing to take the Windows one...

[Build Bot]

Comment on attachment 191067 [details]
Patch

Attachment 191067 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16778896

New failing tests:
editing/selection/selection-invalid-offset.html

[Levi Weintraub]

http://trac.webkit.org/changeset/144646

[Levi Weintraub]

Comment on attachment 191067 [details]
Patch

That buildbot failure is unrelated.