NPAPI plugin metadata (mimetype) should be cached so that loading plugins would be faster. For example, the mimetype -> plugin dynlib mapping should be stored in a file or a sqlite db accompanied with filesystem metadata such as last modified date. Currently plugins that are slow in intialization cause all plugins to be loaded slowly. Also when a plugin initialization causes a crash, every plugin load will end up crashing. Probably could be implemented in QtWebKit by using the platform_strategy similar to this fix: https://bugs.webkit.org/show_bug.cgi?id=40860
info to help to estimate the severity: - a plugin loads in 35ms when no other plugins are present - same plugin loads in 1000ms when 6 other real-world plugins are present
Girish, please feel free to assign the bug to yourself if you get around looking into it :)
The specific problem here is apparently that all plugins are linked to fetch the metadata whether they are used or not. Adding a metadata cache will avoid linking to unused plugins and so slowing down initialization simply by having more plugins. Kimmo said he has a patch.
Created attachment 65413 [details] Implement a cache of NPAPI plugin info using sqlite db
Any measured performance improvements?
What's the motivation of using sqlite as opposed to QSettings or something? May sqlite really is quite lightweight but is it as lightweight as QSettings? Also, do we really need setPluginCachePath() API? Why would one want to change this, plugin cache is just internal optimization isn't it?
Created attachment 65420 [details] Tester to test load-time performance increase Shorter load-time depends on the plugins you have. You can use attached html to dump some values. For me, following sequence gives 500ms difference: export QTWEBKIT_PLUGIN_PATH=$PWD/lib/plugins rm -rf ~/.local/share/data/Nokia/QtTestBrowser/PluginCache.db ./bin/QtTestBrowser ~//swork/meegowrtperf/testcases/html-content/null-plugin-user/plugin-metadata-cache-test.html #inspect "slow case" ./bin/QtTestBrowser ~//swork/meegowrtperf/testcases/html-content/null-plugin-user/plugin-metadata-cache-test.html #inspect "fast case"
(In reply to comment #6) > What's the motivation of using sqlite as opposed to QSettings or something? May sqlite really is quite lightweight but is it as lightweight as QSettings? I chose sqlite mainly because the code could benefit also other ports. If I understand correctly, QSettings does not seem to be suitable for dynamic keys with multiple values. You end up either: - escape the keys (escape '/' char in content) - concatenate the multiple values, (escape the contcatenation separator) Getting the escaping correct seems to me like risk not worth taking.. Sqlite is also quite robust and somewhat resilient to corrupts. I doubt that the actual code would be significantly shorter, so I don't know how much maintainability would be gained. I haven't done any performance or memory footprint measurements between qsettings and sqlite. > Also, do we really need setPluginCachePath() API? Why would one want to change this, plugin cache is just internal optimization isn't it? Yes, this is a matter of API policy.
Comment on attachment 65413 [details] Implement a cache of NPAPI plugin info using sqlite db Generally looks good, some nits: WebCore/ChangeLog:12 + Enabled only for Qt unix flavors currently. Currently only enabled for Qt UNIX flavors. WebCore/plugins/PluginDatabase.cpp:44 + static bool gPersistentCacheIsEnabled; *PluginCache* ? WebCore/plugins/PluginDatabase.cpp:468 + void PluginDatabase::verifySchemaVersion(SQLiteDatabase& database) are you modifying it or why is it not const. In Qt, our coding style tells us to use * in that case to make it more obvious that it is being modified. WebCore/plugins/PluginDatabase.cpp:466 + static const int schemaVersion = 1; unsigned? WebCore/plugins/PluginDatabase.cpp:486 + why two newlines here WebCore/plugins/PluginDatabase.cpp:498 + String completeDatabasePath = pathByAppendingComponent(persistentCachePath(), "PluginCache.db"); absoluteDatabasePath ? WebCore/plugins/PluginDatabase.h:99 + void openDatabase(SQLiteDatabase& database, bool createIfDoesNotExist); We now try to avoid bools like these and use enums instead, personally I think it is ok WebKit/qt/Api/qwebsettings.cpp:1091 + QWebSettings::setPluginCachePath(storagePath); Should we use PluginCache or PluginPersistanceCache ? WebKit/qt/Api/qwebsettings.cpp:1106 + \since 4.X 4.8 :-) WebKit/qt/Api/qwebsettings.cpp:1107 + Sets the path for plugin cache to \a path. Explain this is for persistance, not a cache that the plugins themselves can use. WebKit/qt/Api/qwebsettings.h:122 + static QString pluginCachePath(); const? WebKit/qt/Api/qwebsettings.h:121 + static void setPluginCachePath(const QString &location); & should be left aligned
(In reply to comment #9) > WebKit/qt/Api/qwebsettings.cpp:1091 > + QWebSettings::setPluginCachePath(storagePath); > Should we use PluginCache or PluginPersistanceCache ? > > > WebKit/qt/Api/qwebsettings.cpp:1107 > + Sets the path for plugin cache to \a path. > Explain this is for persistance, not a cache that the plugins themselves can use. > The whole concept should probably be named plugin metadata cache.
Comment on attachment 65413 [details] Implement a cache of NPAPI plugin info using sqlite db Clearing review based on feedback: - change naming to plugin metadata cache - use filesystem directly instead of sqlite
(In reply to comment #6) > What's the motivation of using sqlite as opposed to QSettings or something? May sqlite really is quite lightweight but is it as lightweight as QSettings? > > Also, do we really need setPluginCachePath() API? Why would one want to change this, plugin cache is just internal optimization isn't it? I agree with Girish here. I'm starting to think that it was a mistake to offer other cache paths in the API in the first place, such as the icon database path. In the end it caused more confusion, because people expect things to work out of the box. It is annoying to find out that in order to get for example fav icons - or better startup performance - working you have to set a property to a path value you don't care about. I'd say let's make a smart choice and use it :) If it's a problem we can always add API later. The enablePersistentStorage() API's path could serve as base directory. I don't mind the use of sqlite over QSettings - I see the clear benefit for cross-port usage. In Qt for the plugin cache we use QSettings and it's not a problem AFAIK, i.e. it should be fast enough for these things nowadays. (I say that after the Qtopia guys spend a lot of time optimizing it for the use on their embedded platforms back then)
(In reply to comment #12) > I don't mind the use of sqlite over QSettings - I see the clear benefit for cross-port usage. In Qt for the plugin cache we use QSettings and it's not a problem AFAIK, i.e. it should be fast enough for these things nowadays. (I say that after the Qtopia guys spend a lot of time optimizing it for the use on their embedded platforms back then) One big advantage to me with QSettings is that it's very easy to debug. Plain text files that I can easily change/remove/add. I am OK with sqlite if others feel it's easy to debug with that. Since this is intended to be cross port should we check with other ports?
(In reply to comment #7) > Created an attachment (id=65420) [details] > Tester to test load-time performance increase > > Shorter load-time depends on the plugins you have. > You can use attached html to dump some values. > For me, following sequence gives 500ms difference: > > export QTWEBKIT_PLUGIN_PATH=$PWD/lib/plugins > rm -rf ~/.local/share/data/Nokia/QtTestBrowser/PluginCache.db > > ./bin/QtTestBrowser ~//swork/meegowrtperf/testcases/html-content/null-plugin-user/plugin-metadata-cache-test.html > > #inspect "slow case" > > ./bin/QtTestBrowser ~//swork/meegowrtperf/testcases/html-content/null-plugin-user/plugin-metadata-cache-test.html > > #inspect "fast case" 500ms for just one plugin? That's a good improvement! How did you measure this, using a timestamp or something in the code?
(In reply to comment #8) > If I understand correctly, QSettings does not seem to be suitable for dynamic keys with multiple values. You end up either: > - escape the keys (escape '/' char in content) > - concatenate the multiple values, (escape the contcatenation separator) > > Getting the escaping correct seems to me like risk not worth taking.. These are not really dynamic keys are they? The whole cache is either valid or invalid (and then regenerated). You don't add and remove items. QSettings does seem to support some complex types (lists etc) as the values are QVariants. The primary advantage would be to avoid adding yet another file and format for this performance optimization. All platforms have some sort of settings serialization system all ready so being cross platform doesn't necessarily add that much. I don't have strong opinion about this though.
Created attachment 65548 [details] Plugin metadata cache version 2 with filesystem Changed based on feedback: - "plugin cache" --> "plugin metadata cache" - remove public api to control this - use filesystem to store the info as opposed to sqlite
Comment on attachment 65548 [details] Plugin metadata cache version 2 with filesystem WebCore/plugins/PluginDatabase.cpp:43 + static bool gPersistentPluginMetadataCacheIsEnabled; If you intent this to be for other ports, maybe consider moving it to Settings WebCore/plugins/PluginDatabase.cpp:479 + buffer.resize(bufferCapacity); Is there an upper limit of the cache? WebCore/plugins/PluginDatabase.cpp:526 + // there's error in the cache, we won't try to load it anymore Add dot at the end of comments. WebCore/plugins/PluginDatabase.cpp:534 + LOG_ERROR("Unable to read plugin metadata cache: corrupt schema"); Should we remove the cache then or ? to not let it stay corrupted
Created attachment 65553 [details] Plugin metadata cache version 3 Addresses kenneths comments
(In reply to comment #17) > (From update of attachment 65548 [details]) > WebCore/plugins/PluginDatabase.cpp:43 > + static bool gPersistentPluginMetadataCacheIsEnabled; > If you intent this to be for other ports, maybe consider moving it to Settings Probably could be done if some other port ever starts to use this, and not sooner? > WebCore/plugins/PluginDatabase.cpp:479 > + buffer.resize(bufferCapacity); > Is there an upper limit of the cache? Added limit of 32k. > WebCore/plugins/PluginDatabase.cpp:526 > + // there's error in the cache, we won't try to load it anymore > Add dot at the end of comments. added. > WebCore/plugins/PluginDatabase.cpp:534 > + LOG_ERROR("Unable to read plugin metadata cache: corrupt schema"); > Should we remove the cache then or ? to not let it stay corrupted added.
Comment on attachment 65553 [details] Plugin metadata cache version 3 WebCore/plugins/PluginDatabase.cpp:532 + // there's error in the cache, we won't try to load it anymore.x Seems that an x crept in here :-( WebKit/qt/Api/qwebplugindatabase.cpp:287 + if (plugin->ensurePluginLoaded()) So if someone requests a list of installed plugins, we force load them all? WebCore/plugins/PluginDatabase.h:107 + bool m_persistentMetadataCacheLoaded; I think we normally use the form "IsLoaded" WebCore/plugins/PluginDatabase.cpp:488 + } while (1); while (true) ? :-) Sure you don't want to move the setting to WebCore::Settings?
Created attachment 65690 [details] Plugin metadata cache version 4, fixes a bug with removed plugin directories New patch: - include changes from feedback - add missing check: when reading the cache, only use metadata from the plugins that are from plugin lookup directories. Plugin lookup directory list might change for example by changing QTWEBKIT_PLUGIN_PATH=.
(In reply to comment #20) > (From update of attachment 65553 [details]) > WebCore/plugins/PluginDatabase.cpp:532 > + // there's error in the cache, we won't try to load it anymore.x > Seems that an x crept in here :-( Fixed. > WebKit/qt/Api/qwebplugindatabase.cpp:287 > + if (plugin->ensurePluginLoaded()) > So if someone requests a list of installed plugins, we force load them all? Yes. The plugin load might fail, thus if we want behaviour parity to the cacheless version, we need this. > WebCore/plugins/PluginDatabase.h:107 > + bool m_persistentMetadataCacheLoaded; > I think we normally use the form "IsLoaded" Fixed. > WebCore/plugins/PluginDatabase.cpp:488 > + } while (1); > while (true) ? :-) Fixed. > Sure you don't want to move the setting to WebCore::Settings? I don't have strong feelings about this, but it would seem to make things more complicated until there are more users of this.. Kenneth, can you do still one round of review, since the last patch has changed in behavior? The change is about not using data that comes from plugins outside the plugin directory list.
Comment on attachment 65690 [details] Plugin metadata cache version 4, fixes a bug with removed plugin directories Clearing flags on attachment: 65690 Committed r66297: <http://trac.webkit.org/changeset/66297>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66297 might have broken Qt Windows 32-bit Release
(In reply to comment #23) > (From update of attachment 65690 [details]) > Clearing flags on attachment: 65690 > > Committed r66297: <http://trac.webkit.org/changeset/66297> This change broke the Qt Windows, and Qt Linux Release minimal builds.
It (In reply to comment #26) > (In reply to comment #23) > > (From update of attachment 65690 [details] [details]) > > Clearing flags on attachment: 65690 > > > > Committed r66297: <http://trac.webkit.org/changeset/66297> > > This change broke the Qt Windows, and Qt Linux Release minimal builds. From looking at the stdio logs, the issue is that QWebPluginDatabase::plugins() calls PluginPackage::ensurePluginLoaded(), which is only defined if NETSCAPE_PLUGIN_METADATA_CACHE is enabled. And NETSCAPE_PLUGIN_METADATA_CACHE is not enabled for Windows- and Linux minimal- builds: > @@ -284,7 +284,8 @@ QList<QWebPluginInfo> QWebPluginDatabase::plugins() const > > for (unsigned int i = 0; i < plugins.size(); ++i) { > PluginPackage* plugin = plugins[i]; > - qwebplugins.append(QWebPluginInfo(plugin)); > + if (plugin->ensurePluginLoaded()) > + qwebplugins.append(QWebPluginInfo(plugin)); > } > We should guard the call to PluginPackage::ensurePluginLoaded() with #if ENABLED(NETSCAPE_PLUGIN_METADATA_CACHE) similar to the change that was made in PluginDatabase::pluginForMIMEType() and PluginDatabase::MIMETypeForExtension() in this patch.
Committed build fix in changeset 66307 <http://trac.webkit.org/changeset/66307>.
(In reply to comment #28) > Committed build fix in changeset 66307 <http://trac.webkit.org/changeset/66307>. Thanks Dan!
Revision r66297 cherry-picked into qtwebkit-2.1 with commit 4ed6900e3c028f1a2ace5d2f6d5ae9d7230ea8fe Revision r66307 cherry-picked into qtwebkit-2.1 with commit 8ba35dd2ba984d71a1b91f0111d9eac1a8ce19a2