Bug 129589

Summary: Repatch should plant calls to getters directly rather than through a C helper
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, bunhere, commit-queue, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, rakuco, sam, sergio, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 129778, 130644, 130707    
Bug Blocks: 129588    
Attachments:
Description Flags
it begins
none
more
none
it compiles and instacrashes
none
it compiles and doesn't crash mhahnenberg: review+

Description Filip Pizlo 2014-03-02 17:12:26 PST
...
Comment 1 Filip Pizlo 2014-03-24 23:02:40 PDT
Created attachment 227722 [details]
it begins
Comment 2 Filip Pizlo 2014-03-24 23:18:20 PDT
Created attachment 227724 [details]
more
Comment 3 Filip Pizlo 2014-03-25 11:38:01 PDT
Created attachment 227776 [details]
it compiles and instacrashes
Comment 4 Filip Pizlo 2014-03-25 12:03:54 PDT
Created attachment 227779 [details]
it compiles and doesn't crash

It looks like it's passing tests, but I still have more testing to do - like making sure that it performs and such.
Comment 5 WebKit Commit Bot 2014-03-25 12:42:42 PDT
Attachment 227779 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:370:  Declaration has space between type name and * in alignedNumberOfNeededRegs * sizeof  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/jit/AccessorCallJITStubRoutine.cpp:37:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/AccessorCallJITStubRoutine.cpp:38:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Hahnenberg 2014-03-25 13:02:55 PDT
Comment on attachment 227779 [details]
it compiles and doesn't crash

View in context: https://bugs.webkit.org/attachment.cgi?id=227779&action=review

r=me after running 32-bit tests :-)

> Source/JavaScriptCore/jit/Repatch.cpp:343
> +            // Therefore, we temporary grow the stack for the purpose of the call and then

temporarily

> Source/JavaScriptCore/jit/Repatch.cpp:344
> +            // degrow it after.

s/degrow/shrink
Comment 7 Filip Pizlo 2014-03-25 15:38:01 PDT
Benchmark report for JSRegress on oldmac (MacPro4,1).

VMs tested:
"r166074" at /Volumes/Data/pizlo/secondary/OpenSource/r166074/WebKitBuild/Release/jsc
"ToT" at /Volumes/Data/pizlo/secondary/OpenSource/WebKitBuild/Release/jsc (r166244)
"Getters" at /Volumes/Data/fromMiniMe/primary/OpenSource/WebKitBuild/Release/jsc (r166244)

Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc() between sample measurements.
Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level
timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                               r166074                     ToT                     Getters                Getters v. r166074    

chain-custom-getter       158.2083+-0.1793     ?    160.7440+-19.8519         155.0351+-0.6956        ^ definitely 1.0205x faster
chain-getter-access       279.1823+-8.1286     ?    280.4161+-8.0387     ^     33.7815+-0.4777        ^ definitely 8.2644x faster
getter                    163.8446+-2.9393     ?    168.8126+-5.2703     ^     18.7115+-0.4216        ^ definitely 8.7564x faster
proto-custom-getter       164.6567+-19.4528         161.4323+-0.1810     ?    167.7127+-19.7281       ? might be 1.0186x slower
proto-getter-access       278.1840+-10.8798         275.6373+-8.6379     ^     33.7239+-0.3456        ^ definitely 8.2489x faster
simple-custom-getter      524.4769+-0.5701     !    535.9200+-0.2769     ?    566.0511+-73.2732       ? might be 1.0793x slower
simple-getter-access      461.0245+-2.0153          446.8181+-21.3558    ^     52.0341+-0.4183        ^ definitely 8.8600x faster

<arithmetic>              289.9396+-1.8063     ?    289.9686+-4.5887     ^    146.7214+-9.6163        ^ definitely 1.9761x faster
<geometric> *             259.8404+-3.1578     ?    260.2540+-4.6407     ^     77.1420+-1.2083        ^ definitely 3.3683x faster
<harmonic>                235.1094+-4.3063     ?    235.9726+-5.2733     ^     47.9006+-0.2729        ^ definitely 4.9083x faster
Comment 8 Filip Pizlo 2014-03-25 15:42:22 PDT
Landed in http://trac.webkit.org/changeset/166263, which fails to follow any StickyMH's feedback.  I am following his feedback now.
Comment 9 Filip Pizlo 2014-03-25 16:29:39 PDT
Landed for realzies in http://trac.webkit.org/changeset/166266
Comment 11 Filip Pizlo 2014-03-27 17:34:47 PDT
(In reply to comment #10)
> Caused jscore test failures?
> http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/13373/steps/jscore-test/logs/stdio

Timeouts.  I saw a one-off timeout like that but never reproduced it.  I will skip the test and investigate.

You can skip a JSC test by putting "//@ skip" at the top of it.
Comment 12 Oliver Hunt 2014-03-27 17:38:25 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Caused jscore test failures?
> > http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/13373/steps/jscore-test/logs/stdio
> 
> Timeouts.  I saw a one-off timeout like that but never reproduced it.  I will skip the test and investigate.
> 
> You can skip a JSC test by putting "//@ skip" at the top of it.

The test times out fairly consistently under Debug DRT on my MBP