Bug 79561 - [Qt] REGRESSION: 350% regression in DOM/DOMTable and 2 perf tests time out with newer Qt5
Summary: [Qt] REGRESSION: 350% regression in DOM/DOMTable and 2 perf tests time out wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 79666
  Show dependency treegraph
 
Reported: 2012-02-24 22:59 PST by Ryosuke Niwa
Modified: 2012-03-05 03:20 PST (History)
6 users (show)

See Also:


Attachments
Callgrind output for Qt5 with Jian's patch applied. (4.38 MB, application/octet-stream)
2012-03-01 05:46 PST, Jesus Sanchez-Palencia
no flags Details
Callgrind output for Qt5 WITHOUT Jian's patch applied. (4.38 MB, application/octet-stream)
2012-03-01 05:56 PST, Jesus Sanchez-Palencia
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-02-24 22:59:40 PST
According to perf-o-matic, we have a Qt-specific 250% regression on DOM/DOMTable:
http://trac.webkit.org/log/?rev=108796&stop_rev=108645&verbose=on
Comment 2 Kenneth Rohde Christiansen 2012-02-25 04:49:43 PST
The commits:
http://trac.webkit.org/log/?rev=108796&stop_rev=108645&verbose=on
Comment 3 Csaba Osztrogonác 2012-02-25 05:19:26 PST
Thanks for reporting it, additionally Qt5 update was near r108779. I'll do manual bisecting at moinday to find which revision is the culprit. (Or maybe newer Qt5)
Comment 4 Kenneth Rohde Christiansen 2012-02-25 05:24:54 PST
(In reply to comment #3)
> Thanks for reporting it, additionally Qt5 update was near r108779. I'll do manual bisecting at moinday to find which revision is the culprit. (Or maybe newer Qt5)

That sounds really great!
Comment 5 Csaba Osztrogonác 2012-02-25 07:41:32 PST
I checked it, this regression isn't come from WebKit trunk, but Qt5 update.

- first buildable/working WebKit revision with newer Qt5: http://trac.webkit.org/changeset/108789
- last buildable/working WebKit revision with older Qt5: http://trac.webkit.org/changeset/108785

I checked this two revision, and with newer Qt5:
- Dromaeo/jslib-modify-jquery.html fails with timeout (It makes the perf bot red)
- Parser/html5-full-render.html fails with timeout (It makes the perf bot red)
- DOM/DOMTable.html is ~3.5 times slower
- other test results weren't changed significantly

(( Additionally the previous Qt5 update (not this one, the previous)
made DOM/Accessors.html faster (~800 ms --> ~470 ms) ))
Comment 6 Ryosuke Niwa 2012-02-27 08:01:41 PST
Is Qt statically linked? Or dynamically loaded? Is it updated automatically?
Comment 7 Csaba Osztrogonác 2012-02-27 08:04:57 PST
(In reply to comment #6)
> Is Qt statically linked? Or dynamically loaded? 
Dynamically.

> Is it updated automatically?
No, once a week manually . If everything is OK, on firday.
After update I always send a mail to webkit-qt mailing list.
The latest Qt5 hash is available here: https://github.com/ossy-szeged/qt5-tools/blob/master/build-qt5-env

(And the build script I use to build it.)
Comment 8 Simon Hausmann 2012-02-29 03:43:03 PST
Interesting. If it's such a big difference, it might be possible to spot it in cachegrind, too, between the old qt5 and the newer qt5 snapshot. I suspect a change in qtbase.

Would be great if somebody could run this through a profiler to see if there's anything that sticks out.
Comment 9 Jesus Sanchez-Palencia 2012-03-01 05:20:58 PST
After some profiling I believe the problem is with the following commit in Qt5 (qtbase):

---------------------------------------------------------------
commit 692064bcfd116c2f3a2b30572e511ee68c6a0531
Author: Jiang Jiang <jiang.jiang@nokia.com>
Date:   Wed Feb 15 14:41:07 2012 +0100

    Don't render glyph with FT with fetchMetricsOnly
    
    Neither rendering with outline nor fetchMetricsOnly requires the
    rendering from FreeType so we don't need to render them or cache
    it. It should speed up recalcAdvances() quite a lot.
    
    Change-Id: I0f623cb4f79da2edf6e9c9634a2f22fb0c66823c
    Reviewed-by: Samuel Rødal <samuel.rodal@nokia.com>
---------------------------------------------------------------

I have reverted it locally and the test is back to its regular shape. I will upload callgrind output files that can be used to analyze it soon.

(and I'm really happy that the perf bot helped us to spot this! :)
Comment 10 Jesus Sanchez-Palencia 2012-03-01 05:46:52 PST
Created attachment 129687 [details]
Callgrind output for Qt5 with Jian's patch applied.
Comment 11 Jesus Sanchez-Palencia 2012-03-01 05:56:47 PST
Created attachment 129691 [details]
Callgrind output for Qt5 WITHOUT Jian's patch applied.

As we can see, the whole FontEngine code path is not triggered after reverting the patch.
Comment 12 Simon Hausmann 2012-03-01 07:07:35 PST
Excellent job Jesus! I'll talk to Jiang
Comment 13 Simon Hausmann 2012-03-01 07:29:30 PST
Related Qt change with potential discussion: http://codereview.qt-project.org/#change,16432
Comment 14 Simon Hausmann 2012-03-01 07:49:16 PST
Rollout on the Qt side is in progress http://codereview.qt-project.org/#change,18403 - I suppose this bug can be closed with the next Qt 5 update that brings in this fix (crossing fingers, but likely to miss tomorrow's integration :(
Comment 15 Csaba Osztrogonác 2012-03-01 08:02:23 PST
(In reply to comment #14)
> Rollout on the Qt side is in progress http://codereview.qt-project.org/#change,18403 - I suppose this bug can be closed with the next Qt 5 update that brings in this fix (crossing fingers, but likely to miss tomorrow's integration :(

Should we wait with Qt5 update until rollout lands? I can do 
the update on weekend if the rollout won't be landed tomorrow.
Comment 16 Csaba Osztrogonác 2012-03-05 03:20:03 PST
(In reply to comment #14)
> Rollout on the Qt side is in progress http://codereview.qt-project.org/#change,18403 - I suppose this bug can be closed with the next Qt 5 update that brings in this fix (crossing fingers, but likely to miss tomorrow's integration :(

After Qt5 update everything works again:
https://lists.webkit.org/pipermail/webkit-qt/2012-March/002536.html