Bug 129589 – Repatch should plant calls to getters directly rather than through a C helper

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

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 227722 [details]
it begins

[Filip Pizlo]

Created attachment 227724 [details]
more

[Filip Pizlo]

Created attachment 227776 [details]
it compiles and instacrashes

[Filip Pizlo]

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.

[WebKit Commit Bot]

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.

[Mark Hahnenberg]

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

[Filip Pizlo]

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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/166263, which fails to follow any StickyMH's feedback.  I am following his feedback now.

[Filip Pizlo]

Landed for realzies in http://trac.webkit.org/changeset/166266

[Simon Fraser (smfr)]

Caused jscore test failures?
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/13373/steps/jscore-test/logs/stdio

[Filip Pizlo]

(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.

[Oliver Hunt]

(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