Bug 79561 – [Qt] REGRESSION: 350% regression in DOM/DOMTable and 2 perf tests time out with newer Qt5

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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

[Ryosuke Niwa]

The graph:
http://webkit-perf.appspot.com/graph.html?#tests=[[31016,2001,100137]]&sel=none&displayrange=7&datatype=running

[Kenneth Rohde Christiansen]

The commits:
http://trac.webkit.org/log/?rev=108796&stop_rev=108645&verbose=on

[Csaba Osztrogonác]

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)

[Kenneth Rohde Christiansen]

(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!

[Csaba Osztrogonác]

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

[Ryosuke Niwa]

Is Qt statically linked? Or dynamically loaded? Is it updated automatically?

[Csaba Osztrogonác]

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

[Simon Hausmann]

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.

[Jesus Sanchez-Palencia]

After some profiling I believe the problem is with the following commit in Qt5 (qtbase):

---------------------------------------------------------------
commit 692064bcfd116c2f3a2b30572e511ee68c6a0531
Author: Jiang Jiang <[e-mail]>
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 <[e-mail]>
---------------------------------------------------------------

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! :)

[Jesus Sanchez-Palencia]

Created attachment 129687 [details]
Callgrind output for Qt5 with Jian's patch applied.

[Jesus Sanchez-Palencia]

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.

[Simon Hausmann]

Excellent job Jesus! I'll talk to Jiang

[Simon Hausmann]

Related Qt change with potential discussion: http://codereview.qt-project.org/#change,16432

[Simon Hausmann]

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 :(

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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