Avoid cache when deprecating document.all

Avoid cache when deprecating document.all

To deprecate document.all, simply changing its getter and setter would
activate its cache mechanism, and degrade the performance. Here we
assign it first to 'undefined' to avoid this.

BUG=https://code.google.com/p/chromium/issues/detail?id=239756

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

[Mihai Parparita -not on Chrome, 2013-05-29 15:53:09]

Removing myself as a reviewer, I'm no longer working on Chrome.

[not at google - send to devlin, 2013-05-29 15:58:04]

Mihai you appear to have re-added yourself, but may I humbly suggest that you
change your review alias to something like "Mihai does not work on Chrome" or
similar...

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/platform_app.js:123: document.all =
undefined;
is this the only property that will ever suffer from that problem? if not, it
makes sense to move this into disableGetters.

otherwise lgtm.

[miket_OOO, 2013-05-29 16:19:14]

Waiting for answer from Ben on my question before I approve; I don't think I
understand the entire CL at this point.

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/platform_app.js:123: document.all =
undefined;
On 2013/05/29 15:58:05, kalman wrote:
> is this the only property that will ever suffer from that problem? if not, it
> makes sense to move this into disableGetters.
> 
> otherwise lgtm.

Ben, I'm probably misreading disableGetters (which is a helper I'm not familiar
with), but isn't document.all already there in line 125?

[not at google - send to devlin, 2013-05-29 16:24:30]

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
File chrome/renderer/resources/extensions/platform_app.js (right):

https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
chrome/renderer/resources/extensions/platform_app.js:123: document.all =
undefined;
On 2013/05/29 16:19:14, miket wrote:
> On 2013/05/29 15:58:05, kalman wrote:
> > is this the only property that will ever suffer from that problem? if not,
it
> > makes sense to move this into disableGetters.
> > 
> > otherwise lgtm.
> 
> Ben, I'm probably misreading disableGetters (which is a helper I'm not
familiar
> with), but isn't document.all already there in line 125?

I mean make disableGetters assign every property to undefined ('document',
'aLinkColor', 'all', etc) before setting the getter, rather than only doing it
for document.all.

[miket_OOO, 2013-05-29 16:56:19]

> I mean make disableGetters assign every property to undefined ('document',
> 'aLinkColor', 'all', etc) before setting the getter, rather than only doing it
> for document.all.

Got it. That's a good suggestion. LGTM conditioned on YG's addressing Ben's
comment (happy to re-review if you do implement it).

[Yang Gu, 2013-05-30 16:46:57]

On 2013/05/29 15:58:04, kalman wrote:
> Mihai you appear to have re-added yourself, but may I humbly suggest that you
> change your review alias to something like "Mihai does not work on Chrome" or
> similar...
> 
>
https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
> File chrome/renderer/resources/extensions/platform_app.js (right):
> 
>
https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
> chrome/renderer/resources/extensions/platform_app.js:123: document.all =
> undefined;
> is this the only property that will ever suffer from that problem? if not, it
> makes sense to move this into disableGetters.
> 
> otherwise lgtm.

Thank you very much for the comments! Before this patch, I already checked that
document.all is the only one that uses the cache.
Today I followed your advice and moved the change into disableGetters(). Now
there is still one thing not clear for me. I found some difference between
document.all and the rests (alinkColor, bgColor, fgColor, linkColor,
vlinkColor). Document::all() is defined in Document.cpp (core/dom/), while the
rests are defined in HTMLDocument.cpp (core/html/). After I add
“object[propertyName] = undefined;” into disableGetters(), each time I call
document.all or the rests, e.g., document.vlinkColor, Document::all() will not
be called any more (thus the performance issue is gone), but
HTMLDocument::vlinkColor() would be called.  
Is my way to make them undefined in disableGetters() correct? Is there a way to
avoid calling HTMLDocument::vlinkColor()? Nevertheless, I think a single call
would not have obvious impact on overall performance.

[not at google - send to devlin, 2013-05-31 15:41:25]

On 2013/05/30 16:46:57, Yang Gu wrote:
> On 2013/05/29 15:58:04, kalman wrote:
> > Mihai you appear to have re-added yourself, but may I humbly suggest that
you
> > change your review alias to something like "Mihai does not work on Chrome"
or
> > similar...
> > 
> >
>
https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
> > File chrome/renderer/resources/extensions/platform_app.js (right):
> > 
> >
>
https://codereview.chromium.org/16118002/diff/1/chrome/renderer/resources/ext...
> > chrome/renderer/resources/extensions/platform_app.js:123: document.all =
> > undefined;
> > is this the only property that will ever suffer from that problem? if not,
it
> > makes sense to move this into disableGetters.
> > 
> > otherwise lgtm.
> 
> Thank you very much for the comments! Before this patch, I already checked
that
> document.all is the only one that uses the cache.
> Today I followed your advice and moved the change into disableGetters(). Now
> there is still one thing not clear for me. I found some difference between
> document.all and the rests (alinkColor, bgColor, fgColor, linkColor,
> vlinkColor). Document::all() is defined in Document.cpp (core/dom/), while the
> rests are defined in HTMLDocument.cpp (core/html/). After I add
> “object[propertyName] = undefined;” into disableGetters(), each time I call
> document.all or the rests, e.g., document.vlinkColor, Document::all() will not
> be called any more (thus the performance issue is gone), but
> HTMLDocument::vlinkColor() would be called.  
> Is my way to make them undefined in disableGetters() correct? Is there a way
to
> avoid calling HTMLDocument::vlinkColor()? Nevertheless, I think a single call
> would not have obvious impact on overall performance.

I don't know the answer to this question; I also don't really understand it;
there is also no further uploaded patch so I don't know what your way is.

document.all does seem the most insidious but I can imagine, maybe, the other
things causing a layout or some other kind of unnecessary calculation.

[Yang Gu, 2013-06-04 07:46:32]

I just dig into code of V8 binding, and the whole thing is clear for me. Let me
clarify here. 
Problem:
If we run the case I attached in bug
https://code.google.com/p/chromium/issues/detail?id=239756 with both packaged
app mode and browser mode, there will be a performance difference more than 10%
(packaged app is slower). This case is extracted from Dromaeo dom core tests,
which is a famous benchmark.


Root cause:
The test case is about DOM operations (insertBefore and appendChild). Each time
we add node to DOM in packaged app mode,
Node::invalidateNodeListCachesInAncestors() in Node.cpp
(third_party/WebKit/Source/core/dom/) would spend more time to invalidateCaches
(node->hasRareData() is true). 
Then why node->hasRareData() is true? Because in packaged app mode,  if we
change the getter or setter of document.all in platform_app.js
(chrome/renderer/resources/extensions/), it will actually call Document::all()
in Document.cpp (third_party/WebKit/Source/core/dom/), and enable the cache for
document.all.


Ways to fix:
Way to fix this is to avoid the calling of Document::all(). There are two ways. 
Way 1 (my patch): Just set document.all = undefined;
Way 2 (proposed by kalman): Set all document.[deprecated] = undefined, which
would be as below:
 function disableGetters(object, objectName, propertyNames, opt_messageSuffix) {
   forEach(propertyNames, function(i, propertyName) {
+    object[propertyName] = undefined;
     var stub = generateDisabledMethodStub(objectName + '.' + propertyName,
                                           opt_messageSuffix);

Document::all() is defined in Document.cpp (core/dom/), while the
rests (alinkColor, bgColor, fgColor, linkColor,
vlinkColor) are defined in HTMLDocument.cpp (core/html/). In following
description, I will use document.vlinkColor as an example. According to the
code, only Document::all() employees the cache, while the rests have nothing
related to it. 
According to V8 binding code
{"all", HTMLDocumentV8Internal::allAttrGetterCallback,
HTMLDocumentV8Internal::HTMLDocumentReplaceableAttrSetterCallback, ...}
(gen/webcore/bindings/V8HTMLDocument.cpp)
    HTMLDocumentReplaceableAttrSetterCallback
        HTMLDocumentReplaceableAttrSetter
            ForceSet(v8/include/v8.h)

If we set document.all to undefined (Actually we can set it to any other thing
here), the original hander of Document::all() would be bypassed, so this problem
can be fixed.

But do we need to set all the rests to be undefined, just as the second way?
First of all, the handler of document.vlinkColor has nothing related to cache,
so doing this would not bring any performance gain. Then I thought, if we assign
them to undefined, maybe the original handler would be bypassed (similar to
document.all), and a few cycles might be saved (Assume document.vlinkColor =
undefined consumes few CPU cycles comparing to calling
HTMLDocument::vlinkColor(). I know this is a questionable assumption).
But the document.vlinkColor is different. According to binding code, vlinkColor
is defined as below:
{"vlinkColor", HTMLDocumentV8Internal::vlinkColorAttrGetterCallback,
HTMLDocumentV8Internal::vlinkColorAttrSetterCallback, …}
(gen/webcore/bindings/V8HTMLDocument.cpp)
From the code, we can see vlinkColor's setter doesn't provide the capability of
replacement. So even if we set document.vlinkColor = undefined, next time we
change its getter, HTMLDocument::vlinkColor() will be called again.  

In all, Setting document.all to undefined can bypass Document::all(), thus to
avoid enabling cache when we change its getter, thanks to its replaceable
setter. However, setting the rests to undefined can't make things better, even
that it can't avoid calling related handler defined in HTMLDocument.cpp when we
change their getters.


Test result (There is some environmental change with my machine, so the results
are difference with the ones in bug description. The following data were newly
collected.):
original code in browser mode: 1012.39, 1021.97, 1018.71
original code in packaged app mode: 896.76, 902.68, 905.93
way 1: 1066.35, 1057.05, 1057.09
way 2: 1052.05, 1054.62, 1049.16

The test result also shows there is no much difference between way 1 and 2. So I
think my patch can fix this problem well. 

PS. With my patch, it seems the result of packaged app mode is even slightly
better than browser mode. I don't know the reason.

[not at google - send to devlin, 2013-06-04 15:57:46]

sounds good, let's just do it for document.all then.

[commit-bot: I haz the power, 2013-06-06 02:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yang.gu@intel.com/16118002/1

[Yang Gu, 2013-06-06 02:40:18]

On 2013/06/04 15:57:46, kalman wrote:
> sounds good, let's just do it for document.all then.

Thanks! If you're all OK with the patch, what should I do to land this patch?

[not at google - send to devlin, 2013-06-06 02:43:06]

lgtm, commit queue is fine

[commit-bot: I haz the power, 2013-06-06 13:49:26]

Change committed as 204493