Fix Sunspider performance regression

Fix Sunspider performance regression

For the purpose of updating <webview>'s name attribute, RenderViews report back to the browser process when window.name is mutated. It turns out this hurts Sunspider performance.

This patch introduces a renderer preference that avoids the IPC in regular RenderViews.

BUG=169028
Test=BrowserPluginHostTest.*, Sunspider

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175993

[Fady Samuel, 2013-01-09 21:59:05]

Hi John,

I introduced a performance regression while implementing the name attribute for
<webview>. This patch addresses this. Could you please review?

Thanks,

Fady

[jam, 2013-01-09 22:04:24]

I think creis would be a better reviewer since he reviewed the original change

[Charlie Reis, 2013-01-09 22:10:43]

This doesn't really fix the problem, does it?  It means webview pages would be
14% slower than normal pages on SunSpider, and it means we wouldn't be able to
keep track of window names on normal pages down the road (which we may need to
for other reasons).

Maybe this is a sufficient band-aid to avoid reverting the CL entirely, but are
there other ways we could reduce the performance impact?  Does SunSpider really
change the window name that frequently?

[Fady Samuel, 2013-01-09 22:13:35]

On 2013/01/09 22:10:43, creis wrote:
> This doesn't really fix the problem, does it?  It means webview pages would be
> 14% slower than normal pages on SunSpider, and it means we wouldn't be able to
> keep track of window names on normal pages down the road (which we may need to
> for other reasons).
> 
> Maybe this is a sufficient band-aid to avoid reverting the CL entirely, but
are
> there other ways we could reduce the performance impact?  Does SunSpider
really
> change the window name that frequently?

Unfortunately, yes. I see a massive spew of output if I put a printf in
RenderViewImpl::didChangeName. One solution might be to throttle updates?

[Charlie Reis, 2013-01-09 22:19:46]

On 2013/01/09 22:13:35, Fady Samuel wrote:
> On 2013/01/09 22:10:43, creis wrote:
> > This doesn't really fix the problem, does it?  It means webview pages would
be
> > 14% slower than normal pages on SunSpider, and it means we wouldn't be able
to
> > keep track of window names on normal pages down the road (which we may need
to
> > for other reasons).
> > 
> > Maybe this is a sufficient band-aid to avoid reverting the CL entirely, but
> are
> > there other ways we could reduce the performance impact?  Does SunSpider
> really
> > change the window name that frequently?
> 
> Unfortunately, yes. I see a massive spew of output if I put a printf in
> RenderViewImpl::didChangeName. One solution might be to throttle updates?

Do you mean batch them until the current task finishes?  That might help, but
would that have any bad implications for other windows that are trying to find
it?  e.g., Suppose a page has a name at first, but clears it and then calls
window.open (and sets window.opener to null), hoping that the new window can't
find it via the old name.

[Charlie Reis, 2013-01-09 22:35:06]

If we go forward with this, we should make it clear it's just temporary until we
can find an efficient approach for webview pages.

https://codereview.chromium.org/11833016/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11833016/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:140: // update the
BrowserPlugin's corresponding 'name' attribute.
Please add a TODO with a bug number to remove this.  This isn't a long-term fix,
because it slows down SunSpider-like workloads (whether this many name changes
is realistic in practice or not) within the webview tag, and it means we can't
keep track of window names in normal pages.

https://codereview.chromium.org/11833016/diff/1/content/public/common/rendere...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/11833016/diff/1/content/public/common/rendere...
content/public/common/renderer_preferences.h:112: // process.
Similar TODO.  We don't want this sticking around.

[Fady Samuel, 2013-01-09 22:40:57]

PTAL

+cdn for IPC audit.

https://codereview.chromium.org/11833016/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11833016/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.cc:140: // update the
BrowserPlugin's corresponding 'name' attribute.
On 2013/01/09 22:35:06, creis wrote:
> Please add a TODO with a bug number to remove this.  This isn't a long-term
fix,
> because it slows down SunSpider-like workloads (whether this many name changes
> is realistic in practice or not) within the webview tag, and it means we can't
> keep track of window names in normal pages.

Done.

https://codereview.chromium.org/11833016/diff/1/content/public/common/rendere...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/11833016/diff/1/content/public/common/rendere...
content/public/common/renderer_preferences.h:112: // process.
On 2013/01/09 22:35:06, creis wrote:
> Similar TODO.  We don't want this sticking around.

Done.

[Cris Neckar, 2013-01-09 22:44:00]

IPC security audit LGTM

[Charlie Reis, 2013-01-09 22:46:35]

LGTM.

[commit-bot: I haz the power, 2013-01-09 22:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11833016/9001