Bug 36373 - [Qt] Don't construct a QLineEdit every time when painting a text field
Summary: [Qt] Don't construct a QLineEdit every time when painting a text field
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-03-19 09:40 PDT by Jakub Wieczorek
Modified: 2010-03-29 04:06 PDT (History)
1 user (show)

See Also:


Attachments
patch (3.11 KB, patch)
2010-03-19 09:52 PDT, Jakub Wieczorek
hausmann: review-
Details | Formatted Diff | Diff
patch (4.50 KB, patch)
2010-03-22 08:18 PDT, Jakub Wieczorek
no flags Details | Formatted Diff | Diff
patch (4.50 KB, patch)
2010-03-22 08:21 PDT, Jakub Wieczorek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Wieczorek 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.
Comment 1 Jakub Wieczorek 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 *********
Comment 2 Simon Hausmann 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!
Comment 3 Jakub Wieczorek 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?
Comment 4 Jakub Wieczorek 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.
Comment 5 Simon Hausmann 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? :)
Comment 6 Jakub Wieczorek 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.
Comment 7 Jakub Wieczorek 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.
Comment 8 Jakub Wieczorek 2010-03-22 08:21:13 PDT
Created attachment 51290 [details]
patch

Updated the ChangeLog in respect of the new approach.
Comment 9 Jakub Wieczorek 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>
Comment 10 Jakub Wieczorek 2010-03-22 10:43:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Simon Hausmann 2010-03-29 04:06:12 PDT
Cherry-picked into qtwebkit-4.6 with commit bd724fb2f716336a8a4b54cd2edc96851a5a26a4