Bug 40077 - [Qt] Implement the simple font code path.
: [Qt] Implement the simple font code path.
Status: CLOSED FIXED
: WebKit
Text
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
: QtTriaged
:
:
  Show dependency treegraph
 
Reported: 2010-06-02 12:15 PST by
Modified: 2011-04-19 05:15 PST (History)


Attachments
Preliminary file refactoring patch (34.15 KB, patch)
2010-06-02 12:47 PST, Jocelyn Turcotte
no flags Review Patch | Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2010-06-02 12:51 PST, Jocelyn Turcotte
no flags Review Patch | Details | Formatted Diff | Diff
Preliminary file refactoring patch v2 (8.21 KB, patch)
2010-06-04 01:38 PST, Jocelyn Turcotte
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (19.08 KB, patch)
2010-06-04 01:47 PST, Jocelyn Turcotte
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2010-06-02 12:15:09 PST
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.
------- Comment #1 From 2010-06-02 12:47:26 PST -------
Created an attachment (id=57682) [details]
Preliminary file refactoring patch
------- Comment #2 From 2010-06-02 12:49:45 PST -------
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.
------- Comment #3 From 2010-06-02 12:51:31 PST -------
Created an attachment (id=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.
------- Comment #4 From 2010-06-02 12:55:50 PST -------
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.
------- Comment #5 From 2010-06-02 21:00:44 PST -------
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?
------- Comment #6 From 2010-06-03 03:31:18 PST -------
(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 :)
------- Comment #7 From 2010-06-03 03:41:54 PST -------
(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.
------- Comment #8 From 2010-06-04 01:38:57 PST -------
Created an attachment (id=57852) [details]
Preliminary file refactoring patch v2

Changes:
- No longer move all FontFastPath.cpp but just the code path determination code.
- Fixed style issues
------- Comment #9 From 2010-06-04 01:47:06 PST -------
Created an attachment (id=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.
------- Comment #10 From 2010-06-07 04:00:48 PST -------
(From update of attachment 57852 [details])
This looks good to me. I suggest to wait with landing this until the other patch is also ready for landing.
------- Comment #11 From 2010-06-07 04:29:32 PST -------
(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.
------- Comment #12 From 2010-06-11 02:08:16 PST -------
(From update of attachment 57853 [details])
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.
------- Comment #13 From 2010-06-11 03:03:13 PST -------
(From update of attachment 57852 [details])
Committed r61001: <http://trac.webkit.org/changeset/61001>
------- Comment #14 From 2010-06-11 03:04:04 PST -------
(From update of attachment 57852 [details])
Committed r61002: <http://trac.webkit.org/changeset/61002>
------- Comment #15 From 2010-06-11 03:29:50 PST -------
http://trac.webkit.org/changeset/61002 might have broken Qt Linux ARMv5 Release and Qt Linux ARMv7 Release
------- Comment #16 From 2010-06-11 05:04:23 PST -------
(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.
------- Comment #17 From 2010-06-21 06:06:20 PST -------
<cherry-pick-for-backport: r61005>
------- Comment #18 From 2010-06-21 06:57:26 PST -------
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