Bug 20843 - Accelerated property accesses.
Summary: Accelerated property accesses.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Reported: 2008-09-14 14:51 PDT by Gavin Barraclough
Modified: 2008-09-14 19:37 PDT (History)
0 users

See Also:

The patch (56.67 KB, patch)
2008-09-14 14:53 PDT, Gavin Barraclough
sam: review+
Details | Formatted Diff | Diff
Sunspider results (+2.8%). (5.42 KB, text/plain)
2008-09-14 15:31 PDT, Gavin Barraclough
no flags Details
v8 tests results - ~13% progression. (3.64 KB, text/plain)
2008-09-14 15:37 PDT, Gavin Barraclough
no flags Details
v8 tests results - pre-patch. (3.65 KB, text/plain)
2008-09-14 15:37 PDT, Gavin Barraclough
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2008-09-14 14:51:18 PDT
Inline more of the array access code into the JIT code for get/put_by_val.

Accelerate get/put_by_id by speculatively inlining a disable direct access into the hot path of the code, and repatch this with the correct StructureID and property map offset once these are known.  In the case of accesses to the prototype and reading the array-length a trampoline is genertaed, and the branch to the slow-case is relinked to jump to this.
Comment 1 Gavin Barraclough 2008-09-14 14:53:14 PDT
Created attachment 23416 [details]
The patch
Comment 2 Gavin Barraclough 2008-09-14 15:31:45 PDT
Created attachment 23420 [details]
Sunspider results (+2.8%).
Comment 3 Gavin Barraclough 2008-09-14 15:37:21 PDT
Created attachment 23421 [details]
v8 tests results - ~13% progression.
Comment 4 Gavin Barraclough 2008-09-14 15:37:56 PDT
Created attachment 23422 [details]
v8 tests results - pre-patch.
Comment 5 Sam Weinig 2008-09-14 17:08:04 PDT
Comment on attachment 23416 [details]
The patch

The changelog should have perf numbers as is our norm.

+// This will not sign extend from 8bit, and should be somewhere up in kernel land.
This should be below the includes.  Is this value correct for the mac (bdash thinks perhaps not).  Can it be a const int.

structureStubCompilationInfo should be m_ structureStubCompilationInfo

structIDInstrIdx is an odd variable name.  I think it can be more verbose.

Can these be constants instead of #defines.  Same goes for a few others just like it.

There are a bunch of c-style casts.  Please convert to use c++-style to make cpst happy (and me!).

#if USE(CTI_REPATCH_PIC) && 0 // This is not currently a performance win.  Sad face.
We usually don't commit things like this.

Please replace all TODOs with FIXMEs.

Overall, nothing popped out as being wrong, but it is a lot of code.  I think an extra pair of eyes would be good.  And for the future generations, perhaps a more detailed changelog, explaining exactly what you mean by re-patching.

r=me.  But I think it might be wise to let someone else r+ as well.
Comment 6 Gavin Barraclough 2008-09-14 19:37:59 PDT
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/VM/CTI.cpp
Sending        JavaScriptCore/VM/CTI.h
Sending        JavaScriptCore/VM/CodeBlock.cpp
Sending        JavaScriptCore/VM/CodeBlock.h
Sending        JavaScriptCore/VM/Machine.cpp
Sending        JavaScriptCore/VM/Machine.h
Sending        JavaScriptCore/masm/X86Assembler.h
Transmitting file data ........
Committed revision 36418.