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...
: WebKit
: 528+ (Nightly build)
: Other All
: P2 Enhancement
Assigned To:
: Performance, Qt, QtTriaged
  Show dependency treegraph
Reported: 2009-03-09 13:25 PST by
Modified: 2011-03-27 06:37 PST (History)

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


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

Description From 2009-03-09 13:25:17 PST

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 From 2009-03-09 13:27:54 PST -------
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 From 2009-03-09 13:30:15 PST -------
*** Bug 24469 has been marked as a duplicate of this bug. ***
------- Comment #3 From 2009-03-09 13:31:10 PST -------
Created an attachment (id=28419) [details]
Patch for Qt that adds API to QFont
------- Comment #4 From 2009-03-09 13:32:50 PST -------
Created an attachment (id=28420) [details]
Patch to QtWebKit that takes advantage of new API
------- Comment #5 From 2009-03-10 09:41:47 PST -------
Note: for the QtWebKit patch to apply cleanly, you're webkit branch will need to have applied the change here first:

------- Comment #6 From 2009-03-11 02:15:55 PST -------
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 From 2009-03-27 06:25:07 PST -------
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 From 2009-05-19 20:17:40 PST -------
(From update of attachment 28420 [details])
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 From 2009-06-12 07:46:08 PST -------
Does this have a clear path forward in Qt now?  Any updates?
------- Comment #10 From 2009-10-04 07:34:58 PST -------
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 From 2010-03-24 09:53:48 PST -------
Is this patch still relevant?
------- Comment #12 From 2010-05-13 11:34:25 PST -------
(In reply to comment #11)
> Is this patch still relevant?

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