Bug 21054 - Construction of certain DOM objects is heavily regressed by r36675
Summary: Construction of certain DOM objects is heavily regressed by r36675
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Normal
Assignee: Oliver Hunt
URL: http://nerget.com/jstests/performance...
Depends on:
Reported: 2008-09-24 04:57 PDT by Oliver Hunt
Modified: 2008-09-26 04:56 PDT (History)
6 users (show)

See Also:

patch to fixerate this issue. (26.57 KB, patch)
2008-09-26 04:14 PDT, Oliver Hunt
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 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.
Comment 1 Gavin Sherlock 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
Comment 2 Oliver Hunt 2008-09-24 20:16:35 PDT
Have made a better test (see updated link)

Shark shows this to be StructureID churn 
Comment 3 Oliver Hunt 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.
Comment 4 Oliver Hunt 2008-09-24 22:08:36 PDT

The problem is that JSDOMWindowBase continually recreates the constructor objects for Image, HTMLOptionElement, XHR and Audio, which is basically wrong.
Comment 5 Oliver Hunt 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

Comment 6 Adam Barth 2008-09-25 04:08:51 PDT
As discussed on IRC, this code is also crashy:

Comment 7 Darin Adler 2008-09-25 09:52:02 PDT
I'll help fix this. Just let me know what you'd like me to do.
Comment 8 Oliver Hunt 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)
Comment 9 Maciej Stachowiak 2008-09-26 04:18:11 PDT
Comment on attachment 23847 [details]
patch to fixerate this issue.

Comment 10 Oliver Hunt 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