Bug 43526 – JSArray storage of m_vector is suboptimal

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

The recent shift/unshift speed up employed a trick where JSArray points into the middle of the enclosing ArrayStorage struct by pointing to the contained m_vector element.  This was done for performance reasons as part of a collection of other changes. It turns out that returning to point directly to the ArrayStorage struct performs slightly better (~.3%).

[Michael Saboff]

Created attachment 63521 [details]
Path to change from using m_vector to m_storage

[Geoffrey Garen]

Comment on attachment 63521 [details]
Path to change from using m_vector to m_storage

JavaScriptCore/runtime/JSArray.cpp:851
 +  }
I couldn't figure out why you needed to add this initialization -- it doesn't seem related to just changing what the Array points to. Am I missing something?

JavaScriptCore/runtime/JSArray.h:163
 +          ArrayStorage *m_storage; // Copy of ArrayStorage.m_vector.  Used for quick vector access and to materialize ArrayStorage ptr.
WebKit style guidelines specify "ArrayStorage*" rather than "ArrayStorage *".

I think the comment here is out of date now.

r=me on the meat of the change

[Michael Saboff]

Created attachment 63603 [details]
Updated patch addressing reviewer's concerns

Removed initialization inadvertently included from another patch referenced by reviewer.

Removed comment that no longer applies after change.

[Geoffrey Garen]

Comment on attachment 63603 [details]
Updated patch addressing reviewer's concerns

r=me

[WebKit Commit Bot]

Comment on attachment 63603 [details]
Updated patch addressing reviewer's concerns

Rejecting patch 63603 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
cript-14D857B50A469C100032146C.sh


=== BUILDING AGGREGATE TARGET All OF PROJECT JavaScriptCore WITH CONFIGURATION Debug ===

Checking Dependencies...
** BUILD FAILED **

The following build commands failed:
JavaScriptCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/JavaScriptCore.build/Debug/JavaScriptCore.build/Objects-normal/i386/JSArray.o /Users/eseidel/Projects/CommitQueue/JavaScriptCore/runtime/JSArray.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/3566957

[Michael Saboff]

Created attachment 63742 [details]
Patch that include changes necessitated by patch for bug 43401

Added changes needed when this change is applied after patch 63427 (for bug #43401).

[Geoffrey Garen]

Comment on attachment 63742 [details]
Patch that include changes necessitated by patch for bug 43401

r=me

[Eric Seidel (no email)]

Comment on attachment 63742 [details]
Patch that include changes necessitated by patch for bug 43401

Clearing flags on attachment: 63742

Committed r64937: <http://trac.webkit.org/changeset/64937>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.