Bug 24468 - [Qt] Big performance improvement to FontQt.cpp and hence all text drawing and layouting in QtWebKit
: [Qt] Big performance improvement to FontQt.cpp and hence all text drawing and...
Product: WebKit
Classification: Unclassified
Component: Text
: 528+ (Nightly build)
: Other All
: P2 Enhancement
Assigned To: Nobody
: Performance, Qt, QtTriaged
Depends on:
  Show dependency treegraph
Reported: 2009-03-09 13:25 PDT by Adam Treat
Modified: 2011-03-27 06:37 PDT (History)
10 users (show)

See Also:

Patch for Qt that adds API to QFont (5.11 KB, patch)
2009-03-09 13:31 PDT, Adam Treat
no flags Details | Formatted Diff | Diff
Patch to QtWebKit that takes advantage of new API (9.26 KB, patch)
2009-03-09 13:32 PDT, Adam Treat
staikos: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2009-03-09 13:25:17 PDT

Currently the QtWebKit class FontQt.cpp is using a very expensive method to effectively inform Qt's text engine that it should substitute glyphs for particular unicode characters with another unicode character.  Right now FontQt.cpp is reconstructing a QString based on a TextRun and then detaching and memcpy'ing  whenever it encounters a character that should be treated differently by Qt's text engine according to WebCore::Font::treatAsSpace and WebCore::Font::treatAsZeroWidthSpace.

Profiling shows that this is one of the top ten most expensive methods in all of QtWebKit right now.
Comment 1 Adam Treat 2009-03-09 13:27:54 PDT
The patches attached improve this situation by *never* detaching or recreating a new QString for these operations on a TextRun.

The first patch is a patch to Qt itself that exposes two new methods in the QFont class which allow an application to pass a substitution map to Qt's underlying text engine.  I've only implemented the patch for the Freetype engine in Qt, but other engines could/should be made to honor these substitutions.

The second patch is a patch to FontQt.cpp in WebCore that utilizes the new API in Qfont.

Together these two patches effectively eliminate the cost of the current method of doing things in FontQt.cpp.

Comment 2 Adam Treat 2009-03-09 13:30:15 PDT
*** Bug 24469 has been marked as a duplicate of this bug. ***
Comment 3 Adam Treat 2009-03-09 13:31:10 PDT
Created attachment 28419 [details]
Patch for Qt that adds API to QFont
Comment 4 Adam Treat 2009-03-09 13:32:50 PDT
Created attachment 28420 [details]
Patch to QtWebKit that takes advantage of new API
Comment 5 Adam Treat 2009-03-10 09:41:47 PDT
Note: for the QtWebKit patch to apply cleanly, you're webkit branch will need to have applied the change here first:

Comment 6 Holger Freyther 2009-03-11 02:15:55 PDT
Cool. Is that somehow possible to share the table with Font.cpp without runtime costs? In RenderThemeQt we have a style painter which is setting and clearing the QStyle. Could something like this be used for the substitution hints. 

Something like this http://doc.trolltech.com/4.4/qmutexlocker.html.
Comment 7 Simon Hausmann 2009-03-27 06:25:07 PDT
Instead of the API I think it would be cleaner to make Qt behave the way WebKit expects it to, without the need to modify the string at all. If necessary we could make this configurable through QTextOption or the text flags in the Qt namespace (like TextJustificationForced). I think it should be possible to implement this in a platform independent manner in qtextengine.cpp, see for example QTextEngine::itemize().
Comment 8 George Staikos 2009-05-19 20:17:40 PDT
Comment on attachment 28420 [details]
Patch to QtWebKit that takes advantage of new API

The patch looks fine to me, but apparently Qt Software wants this done differently.  I guess it could be reworked to only apply on older Qt versions up until the one that addresses the issue internally.  Also a changelog is missing.
Comment 9 Adam Treat 2009-06-12 07:46:08 PDT
Does this have a clear path forward in Qt now?  Any updates?
Comment 10 Kenneth Rohde Christiansen 2009-10-04 07:34:58 PDT
What is up with this Simon?

Is there an internal Qt bug report for tracking this? or who is responsible for the Font code in Qt?
Comment 11 Andreas Kling 2010-03-24 09:53:48 PDT
Is this patch still relevant?
Comment 12 Jesus Sanchez-Palencia 2010-05-13 11:34:25 PDT
(In reply to comment #11)
> Is this patch still relevant?

Another Triage and question remains... is this still relevant?
Comment 13 Robert Hogan 2010-11-25 12:05:13 PST
Added to QtWebKit QFont API wishlist at http://bugreports.qt.nokia.com/browse/QTBUG-15627
Comment 14 Andreas Kling 2011-03-27 06:37:14 PDT
We'll be taking another route with the FontFastPath in QtWebKit. See bug 51106 for those developments.