Bug 40077 – [Qt] Implement the simple font code path.

• Component: Text
• Status: CLOSED FIXED

[Jocelyn Turcotte]

Currently QtWebKit always use the complex text code path for text layouting and drawing.

The reason it currently does not use the implementation of the simple text code path already used by other ports is that Qt do not expose individual glyphs information through its API.

[Jocelyn Turcotte]

Created attachment 57682 [details]
Preliminary file refactoring patch

[WebKit Review Bot]

Attachment 57682 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/Font.cpp:295:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:300:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:305:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:310:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:315:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:320:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:325:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:330:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:335:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/graphics/Font.cpp:572:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 10 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Created attachment 57683 [details]
Patch

Note: This patch requires the 032fb3d54eaaa1fa36ec45b37f5f7356b1137830 (Add the Qt::TextBypassShaping flag.) commit to compile against Qt 4.7.
This commit is also not yet in the 4.7 branch.

[Simon Hausmann]

Some additional context for the others:

The "fast path" in WebKit is implemented by caching the character->glyph mappings and the glyph advances in WebKit.

Qt has similar caches, inside the platform specific font engines.

Our goal is to use the mechanism inside WebKit to distinguish between simple and complex text and then use some new APIs that Jocelyn added to Qt 4.7 to layout and draw simple text. For the layout an overload of QFontMetrics::width will do the straight conversion from characters to glyphs and return the sum of the advances - no shaping involved and a very flat call stack with stack allocations in the common case. For drawing the shaping is also skipped and using the existing caches the text is quickly converted to glyphs and then passed down to the paint engine.

With this compromise we hope to achieve the same performance with less overall memory usage by re-using existing Qt caches.

[Holger Freyther]

Here are some comments.

1.) It feels odd to remove the FontFastPath.cpp and then have the new "Qt" simple code directly call the complex text path. This is feeling odd and is not how the code was working. E.g. for Qt < 4.7 the code to decide which fontpath to use, you could simply return Complex....

2.) It is a bit of a step back to introduce PLATFORM(QT) into the Font.cpp code, I have moved out the glyphcache to a new file as we do not want to use it.

3.) Once upon a time I hooked Simon up with the data (google news) for the font code benchmark, would you be interested to update the code and post new numbers?

[Simon Hausmann]

(In reply to comment #5)
> Here are some comments.
> 
> 1.) It feels odd to remove the FontFastPath.cpp and then have the new "Qt" simple code directly call the complex text path. This is feeling odd and is not how the code was working. E.g. for Qt < 4.7 the code to decide which fontpath to use, you could simply return Complex....

Isn't that what Jocelyn's patch does?

I agree it may seem odd, but on the other hand the fast path doesn't have any meaning anymore, right?

> 2.) It is a bit of a step back to introduce PLATFORM(QT) into the Font.cpp code, I have moved out the glyphcache to a new file as we do not want to use it.

That's true, and this time the PLATFORM(QT) is not a temporary hack. Hmm

> 3.) Once upon a time I hooked Simon up with the data (google news) for the font code benchmark, would you be interested to update the code and post new numbers?

Yes, we're interested in re-running benchmarks. Can you send me yours?

The only thing I can find in my inbox regarding google news is the finding that 23% of the incoming characters to floatWidthForComplexText was whitespace :)

[Holger Freyther]

(In reply to comment #6)

> > 1.) It feels odd to remove the FontFastPath.cpp and then have the new "Qt" simple code directly call the complex text path. This is feeling odd and is not how the code was working. E.g. for Qt < 4.7 the code to decide which fontpath to use, you could simply return Complex....
> 
> Isn't that what Jocelyn's patch does?

To some degree... the ugly bit is that the simple path will call the complex one for Qt < 4.7. So for Qt < 4.7 we will end up with

 1.) iterate over the string to determine the path to use
 2.) Call the fast path..
  2.1) Call the complex path.

Counter proposal..
always go through the complex path for Qt < 4.7...


> > 3.) Once upon a time I hooked Simon up with the data (google news) for the font code benchmark, would you be interested to update the code and post new numbers?
> 
> Yes, we're interested in re-running benchmarks. Can you send me yours?

I will have to check... I am pretty sure you had the datastream once, I have to look at my backup.

[Jocelyn Turcotte]

Created attachment 57852 [details]
Preliminary file refactoring patch v2

Changes:
- No longer move all FontFastPath.cpp but just the code path determination code.
- Fixed style issues

[Jocelyn Turcotte]

Created attachment 57853 [details]
Patch v2

Changes:
- Fixed style and compilation issues
- Changed the complex-calls-inside-the-simple-functions to Q_ASSERT(false) to avoid confusion. These function should not be called anyway since we called setCodePath in QWebPagePrivate::QWebPagePrivate()
- Merge the two drawText functions in one function to fix compilation issues. The readability takes a drop but I think it is better than duplicating the text shadow/pen/brush logic.
- gRoundingHackCharacterTable and stuff is still wrapped in "#if !PLATFORM(QT)" to save 256 bytes, I'm not sure it's worth it though.

[Simon Hausmann]

Comment on attachment 57852 [details]
Preliminary file refactoring patch v2

This looks good to me. I suggest to wait with landing this until the other patch is also ready for landing.

[Simon Hausmann]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > > 1.) It feels odd to remove the FontFastPath.cpp and then have the new "Qt" simple code directly call the complex text path. This is feeling odd and is not how the code was working. E.g. for Qt < 4.7 the code to decide which fontpath to use, you could simply return Complex....
> > 
> > Isn't that what Jocelyn's patch does?
> 
> To some degree... the ugly bit is that the simple path will call the complex one for Qt < 4.7. So for Qt < 4.7 we will end up with
> 
>  1.) iterate over the string to determine the path to use
>  2.) Call the fast path..
>   2.1) Call the complex path.
> 
> Counter proposal..
> always go through the complex path for Qt < 4.7...

Right, that's the way to go. Jocelyn's patch does that now by setting the global code path variable on startup for Qt < 4.7.

[Simon Hausmann]

Comment on attachment 57853 [details]
Patch v2

r=me

I suggest to replace "bool complex" to "bool isComplexText" and add a comment on the caller side what the "true" and "false" means.

[Jocelyn Turcotte]

Comment on attachment 57852 [details]
Preliminary file refactoring patch v2

Committed r61001: <http://trac.webkit.org/changeset/61001>

[Jocelyn Turcotte]

Comment on attachment 57852 [details]
Preliminary file refactoring patch v2

Committed r61002: <http://trac.webkit.org/changeset/61002>

[WebKit Review Bot]

http://trac.webkit.org/changeset/61002 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release

[Jocelyn Turcotte]

(In reply to comment #15)
> http://trac.webkit.org/changeset/61002 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release

Build fixes:
Committed r61004: <http://trac.webkit.org/changeset/61004>
Committed r61005: <http://trac.webkit.org/changeset/61005>

Resolving the bug report.

[Simon Hausmann]

<cherry-pick-for-backport: r61005>

[Simon Hausmann]

Revision r61001 cherry-picked into qtwebkit-2.0 with commit ad6104996d73c62d831a04a21fdd8fe5cf30e152
Revision r61002 cherry-picked into qtwebkit-2.0 with commit cbd736c8ff6b45885090910523f23d180b540630
Revision r61004 cherry-picked into qtwebkit-2.0 with commit 95ef651521035cb233e6c8e0ba240dc03db500e0
Revision r61005 cherry-picked into qtwebkit-2.0 with commit 47bd471c833646ff4eac5921df8446b3e60d7927