|
|
Created:
7 years, 7 months ago by Yang Gu Modified:
7 years, 6 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAvoid 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
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (0 generated)
Removing myself as a reviewer, I'm no longer working on Chrome.
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.
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?
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.
> 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).
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.
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.
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.
sounds good, let's just do it for document.all then.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yang.gu@intel.com/16118002/1
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?
lgtm, commit queue is fine
Message was sent while issue was closed.
Change committed as 204493 |