Bug 18208

Summary:

Acid3 test 65 takes >33ms due to plugin refreshing on Windows

Product:

WebKit

Reporter:

Adam Roben (:aroben) <aroben>

Component:

Plug-ins

Assignee:

Adam Roben (:aroben) <aroben>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, drtifti, jhoneycutt

Priority:

Keywords:

HasReduction, PlatformOnly

Version:

528+ (Nightly build)

Hardware:

OS:

Windows XP

URL:

http://acid3.acidtests.org/

Bug Depends on:

18214

Bug Blocks:

17064

Attachments:

Description	Flags
Preliminary cleanup: Make PluginPackage work like our other RefCounted classes	darin: review+
Cleanup 2: Change PlatformFileTime to time_t on Windows	mitz: review+
Cleanup 3: Remove PlatformFileTime	mitz: review+
Reduced testcase	none
Cleanup 4: Rename PluginPaths -> PluginDirectories	mitz: review+
Reduced testcase that works in Firefox as well	none
Cleanup 5: Return const String& from PluginPackage methods	mitz: review+
Cleanup 6: Change getPluginsInDirectories to use an out parameter	mitz: review+
Step 1: Separate filesystem crawling from PluginPackage instantiation	mitz: review+
Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh()	darin: review+
Step 2 (revised)	andersca: review+

Adam Roben (:aroben)

Reported 2008-03-29 10:11:46 PDT

Acid3 test 65 takes 172ms on my MacBook Pro on Windows XP. On Mac the same test takes less than 33ms. The test creates an <object data="svg.xml">, and we seem to spend the majority of the time refreshing the plugin database when we fail to find a plugin to handle text/xml.

Attachments
Preliminary cleanup: Make PluginPackage work like our other RefCounted classes (6.76 KB, patch) 2008-03-29 10:13 PDT, Adam Roben (:aroben)	darin: review+	Details Formatted Diff Diff
Cleanup 2: Change PlatformFileTime to time_t on Windows (5.45 KB, patch) 2008-03-29 10:27 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Cleanup 3: Remove PlatformFileTime (4.15 KB, patch) 2008-03-29 10:27 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Reduced testcase (767 bytes, text/html) 2008-03-29 10:32 PDT, Adam Roben (:aroben)	no flags	Details
Cleanup 4: Rename PluginPaths -> PluginDirectories (20.39 KB, patch) 2008-03-29 10:51 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Reduced testcase that works in Firefox as well (752 bytes, text/html) 2008-03-29 10:52 PDT, Adam Roben (:aroben)	no flags	Details
Cleanup 5: Return const String& from PluginPackage methods (2.01 KB, patch) 2008-03-29 11:16 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Cleanup 6: Change getPluginsInDirectories to use an out parameter (4.69 KB, patch) 2008-03-29 11:25 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Step 1: Separate filesystem crawling from PluginPackage instantiation (15.13 KB, patch) 2008-03-29 23:13 PDT, Adam Roben (:aroben)	mitz: review+	Details Formatted Diff Diff
Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh() (5.25 KB, patch) 2008-03-29 23:13 PDT, Adam Roben (:aroben)	darin: review+	Details Formatted Diff Diff
Step 2 (revised) (5.55 KB, patch) 2008-03-31 09:40 PDT, Adam Roben (:aroben)	andersca: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Adam Roben (:aroben)

Comment 1 2008-03-29 10:13:05 PDT

Created attachment 20192 [details] Preliminary cleanup: Make PluginPackage work like our other RefCounted classes

Darin Adler

Comment 2 2008-03-29 10:22:34 PDT

Comment on attachment 20192 [details] Preliminary cleanup: Make PluginPackage work like our other RefCounted classes r=me

Adam Roben (:aroben)

Comment 3 2008-03-29 10:27:21 PDT

Created attachment 20193 [details] Cleanup 2: Change PlatformFileTime to time_t on Windows

Adam Roben (:aroben)

Comment 4 2008-03-29 10:27:36 PDT

Created attachment 20194 [details] Cleanup 3: Remove PlatformFileTime

Adam Roben (:aroben)

Comment 5 2008-03-29 10:32:01 PDT

Created attachment 20196 [details] Reduced testcase Here's a reduced testcase that just creates the object element and puts it into the document.

mitz

Comment 6 2008-03-29 10:37:42 PDT

Comment on attachment 20193 [details] Cleanup 2: Change PlatformFileTime to time_t on Windows r=me

mitz

Comment 7 2008-03-29 10:38:22 PDT

Comment on attachment 20194 [details] Cleanup 3: Remove PlatformFileTime r=me

Adam Roben (:aroben)

Comment 8 2008-03-29 10:51:16 PDT

Created attachment 20197 [details] Cleanup 4: Rename PluginPaths -> PluginDirectories

Adam Roben (:aroben)

Comment 9 2008-03-29 10:52:23 PDT

Created attachment 20198 [details] Reduced testcase that works in Firefox as well This version of the testcase works in Firefox as well. Firefox doesn't seem to respect the <base> tag when resolving the <object> element's data attribute.

mitz

Comment 10 2008-03-29 10:59:53 PDT

Comment on attachment 20197 [details] Cleanup 4: Rename PluginPaths -> PluginDirectories + void setPluginDirectories(const Vector<String>& paths) { m_pluginDirectories = paths; } You can change the name of the argument here too. -bool PluginDatabase::isPreferredPluginPath(const String& path) +bool PluginDatabase::isPreferredPluginDirectory(const String& path) And here. -static inline void addMozillaPluginPaths(Vector<String>& paths) +static inline void addMozillaPluginDirectories(Vector<String>& paths) And here. -static inline void addWindowsMediaPlayerPluginPath(Vector<String>& paths) +static inline void addWindowsMediaPlayerPluginDirectory(Vector<String>& paths) And in several other places. rs=me either way

Adam Roben (:aroben)

Comment 11 2008-03-29 11:05:44 PDT

(In reply to comment #10) > (From update of attachment 20197 [details] [edit]) > + void setPluginDirectories(const Vector<String>& paths) { > m_pluginDirectories = paths; } > > You can change the name of the argument here too. > > -bool PluginDatabase::isPreferredPluginPath(const String& path) > +bool PluginDatabase::isPreferredPluginDirectory(const String& path) > > And here. > > -static inline void addMozillaPluginPaths(Vector<String>& paths) > +static inline void addMozillaPluginDirectories(Vector<String>& paths) > > And here. > > -static inline void addWindowsMediaPlayerPluginPath(Vector<String>& paths) > +static inline void addWindowsMediaPlayerPluginDirectory(Vector<String>& paths) > > And in several other places. > > rs=me either way I've changed these and more locally. Thanks!

Adam Roben (:aroben)

Comment 12 2008-03-29 11:16:53 PDT

Created attachment 20199 [details] Cleanup 5: Return const String& from PluginPackage methods

mitz

Comment 13 2008-03-29 11:22:25 PDT

Comment on attachment 20199 [details] Cleanup 5: Return const String& from PluginPackage methods r=me

Adam Roben (:aroben)

Comment 14 2008-03-29 11:25:09 PDT

Created attachment 20200 [details] Cleanup 6: Change getPluginsInDirectories to use an out parameter

mitz

Comment 15 2008-03-29 11:32:20 PDT

Comment on attachment 20200 [details] Cleanup 6: Change getPluginsInDirectories to use an out parameter r=me

Adam Roben (:aroben)

Comment 16 2008-03-29 23:13:03 PDT

Created attachment 20209 [details] Step 1: Separate filesystem crawling from PluginPackage instantiation

Adam Roben (:aroben)

Comment 17 2008-03-29 23:13:55 PDT

Created attachment 20210 [details] Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh() I can easily turn attachment 20198 [details] into a regression test, but I worry that it might fail on slower machines. Any suggestions?

mitz

Comment 18 2008-03-29 23:27:27 PDT

Comment on attachment 20209 [details] Step 1: Separate filesystem crawling from PluginPackage instantiation r=me

Darin Adler

Comment 19 2008-03-30 16:38:54 PDT

Comment on attachment 20210 [details] Step 2: Don't do any work if the plugin paths haven't changed between calls to refresh() + HashMap<String, time_t>::iterator found = m_pluginPathsWithTimes.find(*it); Can this just use get instead? r=me

Adam Roben (:aroben)

Comment 20 2008-03-31 09:40:52 PDT

Created attachment 20236 [details] Step 2 (revised) Here's a new version of Step 2. There are two changes from attachment 20210 [details]: 1) I now use HashMap::get instead of HashMap::find, as suggested by Darin in comment 19. 2) We will now handle the case where one or more plugins have been uninstalled but no new plugins have been installed correctly.

Anders Carlsson

Comment 21 2008-03-31 09:42:11 PDT

Comment on attachment 20236 [details] Step 2 (revised) r=me

Adam Roben (:aroben)

Comment 22 2008-03-31 10:00:56 PDT

Committed in r31459, r31460, r31461, r31462, r31463, r31644, r31465, and r31466.

Adam Roben (:aroben)

Comment 23 2008-04-02 13:56:56 PDT

*** Bug 18159 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.