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

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

Summary: Add support for 8 bit TextRuns for ports using HarfBuzz or HarfBuzz NG

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Layout and Rendering (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Levi Weintraub

URL:
Keywords:

Depends on:	97378 100024 110896
Blocks:	111348
	Show dependency tree / graph

Reported:	2012-10-15 17:22 PDT by Levi Weintraub
Modified:	2013-03-04 12:19 PST (History)
CC List:	15 users (show)

See Also:

Attachments
Patch (5.86 KB, patch) 2012-10-15 17:44 PDT, Levi Weintraub	no flags	Details \| Formatted Diff \| Diff
Patch (5.77 KB, patch) 2013-02-25 16:13 PST, Levi Weintraub	no flags	Details \| Formatted Diff \| Diff
Patch for landing (5.19 KB, patch) 2013-02-25 17:08 PST, Levi Weintraub	no flags	Details \| Formatted Diff \| Diff
Patch (5.31 KB, patch) 2013-03-01 16:43 PST, Levi Weintraub	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Levi Weintraub 2012-10-15 17:22:01 PDT

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

Comment 1 Levi Weintraub 2012-10-15 17:44:31 PDT

Created attachment 168822 [details]
Patch

Comment 2 Kenichi Ishibashi 2012-10-15 17:58:33 PDT

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?

Comment 3 Tony Chang 2012-10-16 09:46:48 PDT

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?

Comment 4 Levi Weintraub 2012-10-16 10:04:39 PDT

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

Comment 5 Eric Seidel (no email) 2012-10-16 10:14:09 PDT

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.

Comment 6 Michael Saboff 2012-10-16 10:54:38 PDT

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

Comment 7 Benjamin Poulain 2012-10-16 11:56:48 PDT

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?

Comment 8 Levi Weintraub 2012-10-16 12:10:52 PDT

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

Comment 9 Eric Seidel (no email) 2012-10-22 13:14:03 PDT

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

Comment 10 Eric Seidel (no email) 2012-10-22 17:08:21 PDT

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

Comment 11 Eric Seidel (no email) 2012-10-30 14:52:01 PDT

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

Comment 12 Eric Seidel (no email) 2012-10-30 15:05:27 PDT

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.

Comment 13 Levi Weintraub 2013-02-25 16:13:40 PST

Created attachment 190149 [details]
Patch

Comment 14 Levi Weintraub 2013-02-25 16:14:16 PST

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

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

Comment 15 Eric Seidel (no email) 2013-02-25 16:17:12 PST

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.

Comment 16 Eric Seidel (no email) 2013-02-25 16:17:25 PST

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?

Comment 17 Levi Weintraub 2013-02-25 16:21:03 PST

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

Comment 18 Levi Weintraub 2013-02-25 16:21:37 PST

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

Comment 19 Levi Weintraub 2013-02-25 17:08:50 PST

Created attachment 190160 [details]
Patch for landing

Comment 20 Build Bot 2013-02-25 20:41:30 PST

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

Comment 21 Levi Weintraub 2013-02-25 20:43:24 PST

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

Comment 22 WebKit Review Bot 2013-02-26 09:09:57 PST

Comment on attachment 190160 [details]
Patch for landing

Clearing flags on attachment: 190160

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

Comment 23 WebKit Review Bot 2013-02-26 09:10:03 PST

All reviewed patches have been landed.  Closing bug.

Comment 24 WebKit Review Bot 2013-02-26 11:39:08 PST

Re-opened since this is blocked by bug 110896

Comment 25 Stephen Chenney 2013-02-26 11:46:43 PST

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

Links coming shortly.

Comment 26 Stephen Chenney 2013-02-26 11:48:54 PST

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

Comment 27 Eric Seidel (no email) 2013-02-26 11:50:28 PST

Thanks Stephen

Comment 28 Levi Weintraub 2013-02-26 12:14:17 PST

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]

Comment 29 Levi Weintraub 2013-03-01 14:40:18 PST

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.

Comment 30 Levi Weintraub 2013-03-01 16:43:32 PST

Created attachment 191067 [details]
Patch

Comment 31 Eric Seidel (no email) 2013-03-01 17:01:46 PST

Comment on attachment 191067 [details]
Patch

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

Comment 32 Levi Weintraub 2013-03-01 17:18:33 PST

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

Comment 33 Build Bot 2013-03-01 18:49:23 PST

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

Comment 34 Levi Weintraub 2013-03-04 12:19:46 PST

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

Comment 35 Levi Weintraub 2013-03-04 12:19:56 PST

Comment on attachment 191067 [details]
Patch

That buildbot failure is unrelated.