Bug 21054

Summary: Construction of certain DOM objects is heavily regressed by r36675
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: WebCore JavaScriptAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, gsherloc, mjs, oliver
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://nerget.com/jstests/performance/domtest.html
Attachments:
Description Flags
patch to fixerate this issue. mjs: review+

Oliver Hunt
Reported 2008-09-24 04:57:50 PDT
While Celtic Kane is not a very good benchmark, we should investigate why the ajax tests now take twice as long to run on ToT as they did vs. pre-SFX.
Attachments
patch to fixerate this issue. (26.57 KB, patch)
2008-09-26 04:14 PDT, Oliver Hunt
mjs: review+
Gavin Sherlock
Comment 1 2008-09-24 09:33:28 PDT
It's a regression even since Safari 3.1.2 for the ajax tests. On ToT I get ~80ms, while on 3.1.2 I get 43ms
Oliver Hunt
Comment 2 2008-09-24 20:16:35 PDT
Have made a better test (see updated link) Shark shows this to be StructureID churn
Oliver Hunt
Comment 3 2008-09-24 21:54:03 PDT
Okay in shark a couple of functions look to be of interest: StructureID::~StructureID takes 14% of total time, which implies quite a bit of structureid thrashing StructureID::addPropertyTransition which takes 23% of total time which is due to putDirect("length",..) It should be possible to avoid both of these by initialising these objects with a single shared instance of the StructureID with just a length property. This should hopefully avoid the addPropertyTransition cost, the StructureID alloc and dealloc cost, and reduce the cost of the setting the length property.
Oliver Hunt
Comment 4 2008-09-24 22:08:36 PDT
Progress! The problem is that JSDOMWindowBase continually recreates the constructor objects for Image, HTMLOptionElement, XHR and Audio, which is basically wrong.
Oliver Hunt
Comment 5 2008-09-25 03:39:50 PDT
It turns out that this bug is actually a correctness bug as well, eg. Image.foo = "bar; alert(Image.foo) will alert undefined
Adam Barth
Comment 6 2008-09-25 04:08:51 PDT
As discussed on IRC, this code is also crashy: http://crypto.stanford.edu/~abarth/research/webkit/null-frame/
Darin Adler
Comment 7 2008-09-25 09:52:02 PDT
I'll help fix this. Just let me know what you'd like me to do.
Oliver Hunt
Comment 8 2008-09-26 04:14:25 PDT
Created attachment 23847 [details] patch to fixerate this issue. Fix the bug. Doesn't do the refactoring sam requested as the caching/retrieval function requires knowledge of JSDomWindow and DOMWindow, swhich prevents it from being included in JSDOMBinding.h (it needs to be in the header due to it being a template function)
Maciej Stachowiak
Comment 9 2008-09-26 04:18:11 PDT
Comment on attachment 23847 [details] patch to fixerate this issue. r=me
Oliver Hunt
Comment 10 2008-09-26 04:56:11 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/dom/constructors-cached-expected.txt A LayoutTests/fast/dom/constructors-cached-navigate-expected.txt A LayoutTests/fast/dom/constructors-cached-navigate.html A LayoutTests/fast/dom/constructors-cached.html A LayoutTests/fast/dom/resources/constructors-cached-navigate.js A LayoutTests/fast/dom/resources/constructors-cached.js M WebCore/ChangeLog M WebCore/bindings/js/JSAudioConstructor.cpp M WebCore/bindings/js/JSAudioConstructor.h M WebCore/bindings/js/JSDOMWindowBase.cpp M WebCore/bindings/js/JSDOMWindowBase.h M WebCore/bindings/js/JSHTMLOptionElementConstructor.cpp M WebCore/bindings/js/JSHTMLOptionElementConstructor.h M WebCore/bindings/js/JSImageConstructor.cpp M WebCore/bindings/js/JSImageConstructor.h M WebCore/bindings/js/JSMessageChannelConstructor.cpp M WebCore/bindings/js/JSMessageChannelConstructor.h M WebCore/bindings/js/JSXMLHttpRequestConstructor.cpp M WebCore/bindings/js/JSXMLHttpRequestConstructor.h Committed r36954
Note You need to log in before you can comment on or make changes to this bug.