Summary: | r66992 introduces a perf slowdown in chromium tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | New Bugs | Assignee: | 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
Dirk Pranke
2010-09-10 00:25:14 PDT
Created attachment 67160 [details]
Patch
Comment on attachment 67160 [details]
Patch
ok
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 on attachment 67160 [details] Patch Clearing flags on attachment: 67160 Committed r67178: <http://trac.webkit.org/changeset/67178> All reviewed patches have been landed. Closing bug. Reverted r67178 for reason: re-roll-out patch to put original changes back in Committed r67179: <http://trac.webkit.org/changeset/67179> I’m having difficulty following this. What happened here? Oh, I see comment 3 now. Sorry, this was really confusing. Never heard of anything like this before! 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. 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). (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. 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? (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. Created attachment 67381 [details]
Patch
(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. (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. 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. Som should this be reviewed and landed then? (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. (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. (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." Actually, we were mostly blocked on me verifying that the regression was fixed; Darin's comments were secondary. LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations. |