Bug 45524

Summary: r66992 introduces a perf slowdown in chromium tests
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: aestes, bauerb, darin, eric.carlson, fishd, podivilov, ukai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description Dirk Pranke 2010-09-10 00:25:14 PDT
roll out r66992 and r66997 - possibly introduced a perf slowdown in chromium tests
Comment 1 Dirk Pranke 2010-09-10 00:26:00 PDT
Created attachment 67160 [details]
Patch
Comment 2 Kent Tamura 2010-09-10 00:39:11 PDT
Comment on attachment 67160 [details]
Patch

ok
Comment 3 Dirk Pranke 2010-09-10 00:45:23 PDT
Note that I am checking this in and then immediately rolling it back out, simply so I can point downstream Chromium at revisions nearer to the tip of the tree to test whether or not this change does introduce slowdowns.
Comment 4 Dirk Pranke 2010-09-10 00:45:26 PDT
Comment on attachment 67160 [details]
Patch

Clearing flags on attachment: 67160

Committed r67178: <http://trac.webkit.org/changeset/67178>
Comment 5 Dirk Pranke 2010-09-10 00:45:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Dirk Pranke 2010-09-10 00:48:33 PDT
Reverted r67178 for reason:

re-roll-out patch to put original changes back in

Committed r67179: <http://trac.webkit.org/changeset/67179>
Comment 7 Darin Adler 2010-09-10 08:38:54 PDT
I’m having difficulty following this. What happened here?
Comment 8 Darin Adler 2010-09-10 08:39:26 PDT
Oh, I see comment 3 now. Sorry, this was really confusing. Never heard of anything like this before!
Comment 9 Dirk Pranke 2010-09-10 16:20:01 PDT
re-titling, to capture the actual issue.

It appears that this change triggers a code path on some pages where we end up calling loader()->resourceWillUsePlugin() repeatedly during updateWidget(), and resourceWillUsePlugin() can potentially be an expensive call (e.g., it may hit the registry on Windows).

It seems like this check should be cacheable and only be invalidated if the DOM state mutates, and that would eliminate the performance hit.
Comment 10 Dirk Pranke 2010-09-10 16:25:17 PDT
Note that this shows up in Chromium as an increase in the I/O operations between the renderer process and the browser process (since looking up values in the registry has to be routed through the browser via sync IPCs).
Comment 11 Andy Estes 2010-09-10 16:26:54 PDT
(In reply to comment #9)
> re-titling, to capture the actual issue.
> 
> It appears that this change triggers a code path on some pages where we end up calling loader()->resourceWillUsePlugin() repeatedly during updateWidget(), and resourceWillUsePlugin() can potentially be an expensive call (e.g., it may hit the registry on Windows).
> 
> It seems like this check should be cacheable and only be invalidated if the DOM state mutates, and that would eliminate the performance hit.

Interesting! I was expecting the regression was due to rendering fallback content, not checking MIME types. Thanks for that critical piece of info.
Comment 12 Andy Estes 2010-09-10 19:16:41 PDT
It looks like Safari on Windows caches MIME types in MIMETypeRegistry::getMIMETypeForExtension() (see http://trac.webkit.org/browser/trunk/WebCore/platform/win/MIMETypeRegistryWin.cpp#L65). Perhaps chromium needs to do the same thing in the web process that it checks before calling over to the UI process. Thoughts?
Comment 13 Darin Fisher (:fishd, Google) 2010-09-11 22:18:00 PDT
(In reply to comment #12)
> It looks like Safari on Windows caches MIME types in MIMETypeRegistry::getMIMETypeForExtension() (see http://trac.webkit.org/browser/trunk/WebCore/platform/win/MIMETypeRegistryWin.cpp#L65). Perhaps chromium needs to do the same thing in the web process that it checks before calling over to the UI process. Thoughts?

Are you sure it is wise to cache system mime mappings?  What if they change?  Should the browser need to be restarted after the user installs some other random piece of software on their system?

While caching at this layer masks the problem, it seems to suffer from correctness issues.
Comment 14 Andy Estes 2010-09-13 02:19:16 PDT
Created attachment 67381 [details]
Patch
Comment 15 Andy Estes 2010-09-13 02:21:41 PDT
(In reply to comment #14)
> Created an attachment (id=67381) [details]
> Patch

Hopefully this fixes the performance regression. Dirk, if possible, you should confirm that before it gets r+'d.
Comment 16 Andy Estes 2010-09-13 02:24:33 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > It looks like Safari on Windows caches MIME types in MIMETypeRegistry::getMIMETypeForExtension() (see http://trac.webkit.org/browser/trunk/WebCore/platform/win/MIMETypeRegistryWin.cpp#L65). Perhaps chromium needs to do the same thing in the web process that it checks before calling over to the UI process. Thoughts?
> 
> Are you sure it is wise to cache system mime mappings?  What if they change?  Should the browser need to be restarted after the user installs some other random piece of software on their system?
> 
> While caching at this layer masks the problem, it seems to suffer from correctness issues.

Good point; that's a drawback I didn't consider.
Comment 17 Dirk Pranke 2011-05-31 18:55:31 PDT
clearing ownership, in case someone else would like to take a look at this. I'm not planning on looking at it any time soon.
Comment 18 Alexey Proskuryakov 2011-06-02 00:15:59 PDT
Som should this be reviewed and landed then?
Comment 19 Dirk Pranke 2011-06-02 10:52:20 PDT
(In reply to comment #18)
> Som should this be reviewed and landed then?

If I recall correctly, Andy didn't address Darin's concerns about correctness. And, although I'm not sure if we'd be able easily verify the performance regression at this point, we should probably still try.
Comment 20 Andy Estes 2011-06-02 11:36:56 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > Som should this be reviewed and landed then?
> 
> If I recall correctly, Andy didn't address Darin's concerns about correctness. And, although I'm not sure if we'd be able easily verify the performance regression at this point, we should probably still try.

Unfortunately I accepted Darin's comment at face value back in September without really evaluating it. Thinking about it now, I think the level at which the MIME type is cached is correct. The cache has the lifetime of the <embed> or <object> DOM node and is dirtied whenever the node's subtree is modified or attributes affecting plug-in loading are changed. The cache does not have the lifetime of the browser; changes in system MIME type mappings would not require a browser restart.

I think I was waiting to hear if this patch resolved the performance regression, and chromium folks were waiting for me to solve the perf regression. Sorry for the poor communication.

If we think there is still a regression, I'm happy to get this patch landed.
Comment 21 Andy Estes 2011-06-02 11:38:25 PDT
(In reply to comment #20)
> I think I was waiting to hear if this patch resolved the performance regression, and chromium folks were waiting for me to solve the perf regression. Sorry for the poor communication.

This should read "...and the chromium folks were waiting for me to fix the perceived correctness issue."
Comment 22 Dirk Pranke 2011-06-02 16:58:57 PDT
Actually, we were mostly blocked on me verifying that the regression was fixed; Darin's comments were secondary.
Comment 23 Stephen Chenney 2013-04-09 16:10:09 PDT
LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.