Bug 45524 – r66992 introduces a perf slowdown in chromium tests

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dirk Pranke]

roll out r66992 and r66997 - possibly introduced a perf slowdown in chromium tests

[Dirk Pranke]

Created attachment 67160 [details]
Patch

[Kent Tamura]

Comment on attachment 67160 [details]
Patch

ok

[Dirk Pranke]

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.

[Dirk Pranke]

Comment on attachment 67160 [details]
Patch

Clearing flags on attachment: 67160

Committed r67178: <http://trac.webkit.org/changeset/67178>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Reverted r67178 for reason:

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

Committed r67179: <http://trac.webkit.org/changeset/67179>

[Darin Adler]

I’m having difficulty following this. What happened here?

[Darin Adler]

Oh, I see comment 3 now. Sorry, this was really confusing. Never heard of anything like this before!

[Dirk Pranke]

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.

[Dirk Pranke]

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

[Andy Estes]

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

[Andy Estes]

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?

[Darin Fisher (:fishd, Google)]

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

[Andy Estes]

Created attachment 67381 [details]
Patch

[Andy Estes]

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

[Andy Estes]

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

[Dirk Pranke]

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.

[Alexey Proskuryakov]

Som should this be reviewed and landed then?

[Dirk Pranke]

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

[Andy Estes]

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

[Andy Estes]

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

[Dirk Pranke]

Actually, we were mostly blocked on me verifying that the regression was fixed; Darin's comments were secondary.

[Stephen Chenney]

LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.