Bug 24468 – [Qt] Big performance improvement to FontQt.cpp and hence all text drawing and layouting in QtWebKit

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Text

[Adam Treat]

Hi,

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.

[Adam Treat]

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

Cheers,
Adam

[Adam Treat]

*** Bug 24469 has been marked as a duplicate of this bug. ***

[Adam Treat]

Created attachment 28419 [details]
Patch for Qt that adds API to QFont

[Adam Treat]

Created attachment 28420 [details]
Patch to QtWebKit that takes advantage of new API

[Adam Treat]

Note: for the QtWebKit patch to apply cleanly, you're webkit branch will need to have applied the change here first:

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

[Holger Freyther]

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.

[Simon Hausmann]

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

[George Staikos]

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.

[Adam Treat]

Does this have a clear path forward in Qt now?  Any updates?

[Kenneth Rohde Christiansen]

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?

[Andreas Kling]

Is this patch still relevant?

[Jesus Sanchez-Palencia]

(In reply to comment #11)
> Is this patch still relevant?

Another Triage and question remains... is this still relevant?

[Robert Hogan]

Added to QtWebKit QFont API wishlist at http://bugreports.qt.nokia.com/browse/QTBUG-15627

[Andreas Kling]

We'll be taking another route with the FontFastPath in QtWebKit. See bug 51106 for those developments.