Bug 43526 - JSArray storage of m_vector is suboptimal
Summary: JSArray storage of m_vector is suboptimal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-04 17:24 PDT by Michael Saboff
Modified: 2010-08-07 22:29 PDT (History)
2 users (show)

See Also:


Attachments
Path to change from using m_vector to m_storage (39.27 KB, patch)
2010-08-04 17:46 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff
Updated patch addressing reviewer's concerns (38.94 KB, patch)
2010-08-05 10:17 PDT, Michael Saboff
ggaren: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch that include changes necessitated by patch for bug 43401 (39.14 KB, patch)
2010-08-06 11:01 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2010-08-04 17:24:36 PDT
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%).
Comment 1 Michael Saboff 2010-08-04 17:46:45 PDT
Created attachment 63521 [details]
Path to change from using m_vector to m_storage
Comment 2 Geoffrey Garen 2010-08-04 21:46:12 PDT
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
Comment 3 Michael Saboff 2010-08-05 10:17:46 PDT
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.
Comment 4 Geoffrey Garen 2010-08-05 11:31:11 PDT
Comment on attachment 63603 [details]
Updated patch addressing reviewer's concerns

r=me
Comment 5 WebKit Commit Bot 2010-08-06 01:18:14 PDT
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
Comment 6 Michael Saboff 2010-08-06 11:01:13 PDT
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).
Comment 7 Geoffrey Garen 2010-08-06 13:18:21 PDT
Comment on attachment 63742 [details]
Patch that include changes necessitated by patch for bug 43401

r=me
Comment 8 Eric Seidel (no email) 2010-08-07 22:29:46 PDT
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>
Comment 9 Eric Seidel (no email) 2010-08-07 22:29:52 PDT
All reviewed patches have been landed.  Closing bug.