Bug 36373

Summary: [Qt] Don't construct a QLineEdit every time when painting a text field
Product: WebKit Reporter: Jakub Wieczorek <jwieczorek>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
hausmann: review-
patch
none
patch none

Jakub Wieczorek
Reported 2010-03-19 09:40:25 PDT
In RenderThemeQt.cpp there is this magic findFrameLineWidth() function, which queries the style for the frame line width: static int findFrameLineWidth(QStyle* style) { QLineEdit lineEdit; QStyleOptionFrameV2 opt; return style->pixelMetric(QStyle::PM_DefaultFrameWidth, &opt, &lineEdit); } It needs to provide the style with a widget so that the style will give the right width, but constructing it every time seems like an overkill. Making it static and keeping one instance in the memory should not hurt the memory consumption too much.
Attachments
patch (3.11 KB, patch)
2010-03-19 09:52 PDT, Jakub Wieczorek
hausmann: review-
patch (4.50 KB, patch)
2010-03-22 08:18 PDT, Jakub Wieczorek
no flags
patch (4.50 KB, patch)
2010-03-22 08:21 PDT, Jakub Wieczorek
no flags
Jakub Wieczorek
Comment 1 2010-03-19 09:52:41 PDT
Created attachment 51163 [details] patch Additionally I included a very basic benchmark I used to measure the gain. I'm not sure if these results are trustworthy but they are the following. According to Valgrind, quite a bit of time is spent in QLineEditPrivate::init(). Before: ********* Start testing of tst_Painting ********* Config: Using QTest library 4.7.0, Qt 4.7.0 PASS : tst_Painting::initTestCase() RESULT : tst_Painting::textAreas(): 62.415 msecs per iteration (total: 62,416, iterations: 1000) PASS : tst_Painting::textAreas() PASS : tst_Painting::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped ********* Finished testing of tst_Painting ********* After: ********* Start testing of tst_Painting ********* Config: Using QTest library 4.7.0, Qt 4.7.0 PASS : tst_Painting::initTestCase() RESULT : tst_Painting::textAreas(): 46.996 msecs per iteration (total: 46,996, iterations: 1000) PASS : tst_Painting::textAreas() PASS : tst_Painting::cleanupTestCase() Totals: 3 passed, 0 failed, 0 skipped ********* Finished testing of tst_Painting *********
Simon Hausmann
Comment 2 2010-03-21 15:40:43 PDT
Comment on attachment 51163 [details] patch Good catch Jakub!! It really sucks that we have to create a QLineEdit instance in the first place :-(, but I understand that this is a flaw in the QStyle API. Instead of creating a global QLineEdit that runs the risk of being destructed after the QApplication destructor, how about assocating it with the RenderTheme? Then it would be destroyed when the page dies. Alternatively you could _cache_ the frame width in the RenderThemeQt as a member variable and initialize it through a temporary QLineEdit instance the first time it's queried. It's not perfect as it wouldn't follow style changes, but it's a start :) I'm going to say r- because I believe we should destroy the QLineEdit instance earlier. But otherwise a great catch!
Jakub Wieczorek
Comment 3 2010-03-21 15:48:33 PDT
(In reply to comment #2) > (From update of attachment 51163 [details]) > Good catch Jakub!! > > It really sucks that we have to create a QLineEdit instance in the first place > :-(, but I understand that this is a flaw in the QStyle API. > > Instead of creating a global QLineEdit that runs the risk of being destructed > after the QApplication destructor, how about assocating it with the > RenderTheme? Then it would be destroyed when the page dies. With this patch, QLineEdit would not be destructed as such - DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not be called at the exit. > Alternatively you could _cache_ the frame width in the RenderThemeQt as a > member variable and initialize it through a temporary QLineEdit instance the > first time it's queried. Would this work with style changes during the application lifetime?
Jakub Wieczorek
Comment 4 2010-03-21 15:49:18 PDT
(In reply to comment #3) > Would this work with style changes during the application lifetime? Nevermind that. I just noticed your comment on that.
Simon Hausmann
Comment 5 2010-03-21 16:09:17 PDT
(In reply to comment #3) > > Instead of creating a global QLineEdit that runs the risk of being destructed > > after the QApplication destructor, how about assocating it with the > > RenderTheme? Then it would be destroyed when the page dies. > > With this patch, QLineEdit would not be destructed as such - > DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not > be called at the exit. True true, but that's evil, too, isn't it? :)
Jakub Wieczorek
Comment 6 2010-03-21 16:37:06 PDT
(In reply to comment #5) > (In reply to comment #3) > > > Instead of creating a global QLineEdit that runs the risk of being destructed > > > after the QApplication destructor, how about assocating it with the > > > RenderTheme? Then it would be destroyed when the page dies. > > > > With this patch, QLineEdit would not be destructed as such - > > DEFINE_STATIC_LOCAL allocates an object on the heap so the destructor would not > > be called at the exit. > > True true, but that's evil, too, isn't it? :) Right. :) Another option would be to make the QApplication instance the parent of this object. This way we could avoid having one QLineEdit per page and could keep up with the style change. If that sounds reasonable, I'll try this tomorrow, otherwise I'll pick either of the ideas suggested by you.
Jakub Wieczorek
Comment 7 2010-03-22 08:18:34 PDT
Created attachment 51289 [details] patch OK, I went for having a QLineEdit per RenderTheme to simplify dealing with its lifetime. This could be further improved if needed.
Jakub Wieczorek
Comment 8 2010-03-22 08:21:13 PDT
Created attachment 51290 [details] patch Updated the ChangeLog in respect of the new approach.
Jakub Wieczorek
Comment 9 2010-03-22 10:42:55 PDT
Comment on attachment 51290 [details] patch Clearing flags on attachment: 51290 Committed r56343: <http://trac.webkit.org/changeset/56343>
Jakub Wieczorek
Comment 10 2010-03-22 10:43:04 PDT
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 11 2010-03-29 04:06:12 PDT
Cherry-picked into qtwebkit-4.6 with commit bd724fb2f716336a8a4b54cd2edc96851a5a26a4
Note You need to log in before you can comment on or make changes to this bug.