Bug 106140 - [Qt] Major performance improvement in Qt's PluginDatabase implementation
Summary: [Qt] Major performance improvement in Qt's PluginDatabase implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on: 106223 106463
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-04 15:20 PST by David Faure
Modified: 2013-01-30 02:36 PST (History)
5 users (show)

See Also:


Attachments
the patch (2.16 KB, patch)
2013-01-05 03:40 PST, David Faure
no flags Details | Formatted Diff | Diff
the patch with changelog added (2.52 KB, patch)
2013-01-06 02:40 PST, David Faure
no flags Details | Formatted Diff | Diff
new patch, with refcounting unchanged (3.02 KB, patch)
2013-01-12 01:31 PST, David Faure
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Faure 2013-01-04 15:20:49 PST
load() calls NP_Initialize, which is very slow and even leads to nppdf.so
showing the acrobat reader license dialog.

fetchInfo() doesn't need to call all that code, it only needs to load the QLibrary.
Comment 1 David Faure 2013-01-05 03:40:42 PST
Created attachment 181434 [details]
the patch

Now with the patch, oops.

I'm wondering if my approach is valid: googling for NP_Initialize and NP_GetValue API docs seems to indicate that the browser is supposed to call NP_Initialize before NP_GetValue? It appears to work fine, though, all plugins I have here return correct stuff (description and mimetypes) from NP_GetValue even without calling NP_Initialize.


Note that I had to remove "m_infoIsFromCache = false" so that ensurePluginLoaded() doesn't return m_isLoaded (which is false at that point, when called from PluginDatabase::pluginForMIMEType). Basically this is what PluginPackageWin.cpp does as well (not calling load from fetchInfo, and not setting m_infoIsFromCache to false), while PluginPackageMac.cpp calls load from fetchInfo and sets that bool to false (like the Qt code was doing before this patch). So I presume this is correct (it does fix the actual loading of plugins), even though I don't fully understand the m_infoIsFromCache logic.
Comment 2 Alexis Menard (darktears) 2013-01-05 04:21:46 PST
Hi David,

You need to run the script in Tools/Script/prepare-ChangeLog -b 106140 to generate the ChangeLog.

And then you can request the review and commit queue flag to ?
Comment 3 Simon Hausmann 2013-01-05 09:08:27 PST
Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?
Comment 4 David Faure 2013-01-06 02:40:28 PST
Created attachment 181456 [details]
the patch with changelog added
Comment 5 David Faure 2013-01-06 02:48:43 PST
(In reply to comment #3)
> Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?

The Windows implementation already does this.
The Mac implementation could do pretty much the same as this patch (call CFBundleCreate directly in fetchInfo), but I can't test that, so better if a Mac guy does that. As you can see, no change is required in the shared code, so implementations can do this optimization independently from each other.
Comment 6 Simon Hausmann 2013-01-06 07:52:23 PST
(In reply to comment #5)
> (In reply to comment #3)
> > Good idea :) any chance of sharing this logic/optimization with the other plugin package implementations?
> 
> The Windows implementation already does this.
> The Mac implementation could do pretty much the same as this patch (call CFBundleCreate directly in fetchInfo), but I can't test that, so better if a Mac guy does that. As you can see, no change is required in the shared code, so implementations can do this optimization independently from each other.

I was thinking of Gtk and EFL :)
Comment 7 David Faure 2013-01-07 00:19:19 PST
Ah. Same issue: I can make a very similar patch, but I can't test it.


Also, I just realized that with this patch, m_infoIsFromCache is now completely unused (it's always true, for all implementations). I can add the cleanup of that bool to the patch.
Comment 8 Simon Hausmann 2013-01-07 07:34:42 PST
Comment on attachment 181456 [details]
the patch with changelog added

r=me

Yeah, sounds like the cleanup of m_infoIsFromCache should be a separate cleanup patch.

Other suggestions:

    * Move the QLibrary initializing code into a separate shared help method instead of duplicating the code?
    * Make PluginPackage::fetchInfo() shared code between Qt, Gtk and EFL. There's little reason I think for this code to be duplicated and if shared the other platforms could benefit from the same optimization.
Comment 9 WebKit Review Bot 2013-01-07 07:41:40 PST
Comment on attachment 181456 [details]
the patch with changelog added

Clearing flags on attachment: 181456

Committed r138944: <http://trac.webkit.org/changeset/138944>
Comment 10 WebKit Review Bot 2013-01-07 07:41:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogonác 2013-01-07 08:31:34 PST
(In reply to comment #9)
> (From update of attachment 181456 [details])
> Clearing flags on attachment: 181456
> 
> Committed r138944: <http://trac.webkit.org/changeset/138944>

It made 28 tests crash - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20NRWT/builds/26195

Could you check it?
Comment 12 WebKit Review Bot 2013-01-07 08:38:02 PST
Re-opened since this is blocked by bug 106223
Comment 13 Csaba Osztrogonác 2013-01-07 08:43:32 PST
(In reply to comment #12)
> Re-opened since this is blocked by bug 106223

rollout landed in http://trac.webkit.org/changeset/138948
Comment 14 Simon Hausmann 2013-01-07 12:50:07 PST
Thanks for the rollout, Ossy
Comment 15 David Faure 2013-01-09 01:17:24 PST
I can't reproduce the crashes.
http://paste.kde.org/641462/
http://paste.kde.org/641486/
I tried a number of others (from the list at https://gist.github.com/4483529), but they all run fine (except LayoutTests/http/tests/plugins/get-url.html because I don't have apache installed).


I suspect that the CI machine has different plugins installed than I do. How can I find more information about which plugins it has... and/or how can I get a full backtrace in debug mode, from the CI machine?
Or, maybe someone else can try the patch locally to see if he manages to get a crash?


Simon: of course I thought about factorizing the code, but that's a bit messy with the header being shared with other implementations. I'd have to add the method declaration in the .h inside some sort ifdef QT, or I would have to make a file-static helper function but then it can't access members, making the code ugly.

For the sharing with other impls, that's rather unrelated to this patch (could have been done before, too). Let's come back to that once the basic patch is in...
Comment 16 Csaba Osztrogonác 2013-01-09 06:57:22 PST
Tests pass if you run only one, but many of them crash if you run all plugins
directory. I tried r138944 in debug mode and I got crash after the 4. plugins
test:

LayoutTests/platform/qt/plugins/qt-qwidget-plugin.html
LayoutTests/plugins/access-after-page-destroyed.html
LayoutTests/plugins/attach-during-destroy.html
LayoutTests/plugins/change-widget-and-click-crash.html

And here is the GDB bactracke with all necessary information:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff4545d98 in WebCore::PluginPackage::freeLibraryTimerFired (this=0x6a44a0) at /home/oszi/WebKit/Source/WebCore/plugins/PluginPackage.cpp:70
70          ASSERT(!m_loadCount);
(gdb) bt
#0  0x00007ffff4545d98 in WebCore::PluginPackage::freeLibraryTimerFired (this=0x6a44a0) at /home/oszi/WebKit/Source/WebCore/plugins/PluginPackage.cpp:70
#1  0x00007ffff4547256 in WebCore::Timer<WebCore::PluginPackage>::fired (this=0x6a4790) at /home/oszi/WebKit/Source/WebCore/platform/Timer.h:106
#2  0x00007ffff4531fe9 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x6bbbd0) at /home/oszi/WebKit/Source/WebCore/platform/ThreadTimers.cpp:116
#3  0x00007ffff4531f13 in WebCore::ThreadTimers::sharedTimerFired () at /home/oszi/WebKit/Source/WebCore/platform/ThreadTimers.cpp:93
#4  0x00007ffff488fd80 in WebCore::SharedTimerQt::timerEvent (this=0x6bbc00, ev=0x7fffffffdd50) at /home/oszi/WebKit/Source/WebCore/platform/qt/SharedTimerQt.cpp:113
#5  0x00007fffe8d49439 in QObject::event(QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#6  0x00007fffea3db8cc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5
#7  0x00007fffea3e1beb in QApplication::notify(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5
#8  0x00007fffe8d24c04 in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#9  0x00007fffe8d700f2 in QTimerInfoList::activateTimers() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#10 0x00007fffe8d70b5d in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#11 0x00007fffec1ee6f2 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#12 0x00007fffec1f2568 in ?? () from /lib/libglib-2.0.so.0
#13 0x00007fffec1f271c in g_main_context_iteration () from /lib/libglib-2.0.so.0
#14 0x00007fffe8d7081b in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#15 0x00007fffe8d23e4b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#16 0x00007fffe8d29e4d in QCoreApplication::exec() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5
#17 0x0000000000431d54 in main (argc=2, argv=0x7fffffffe3b8) at /home/oszi/WebKit/Tools/DumpRenderTree/qt/DumpRenderTreeMain.cpp:203
Comment 17 David Faure 2013-01-12 01:31:30 PST
Created attachment 182463 [details]
new patch, with refcounting unchanged

The assert has been fixed in bug 106463. There was another issue, the unloading of flash actually crashes. This new patch solves this.
Comment 18 David Faure 2013-01-29 09:58:01 PST
Alexis, Simon: ping?

(Patch is ready for review)
Comment 19 WebKit Review Bot 2013-01-30 02:35:56 PST
Comment on attachment 182463 [details]
new patch, with refcounting unchanged

Clearing flags on attachment: 182463

Committed r141240: <http://trac.webkit.org/changeset/141240>
Comment 20 WebKit Review Bot 2013-01-30 02:36:01 PST
All reviewed patches have been landed.  Closing bug.