Bug 129589 - Repatch should plant calls to getters directly rather than through a C helper
Summary: Repatch should plant calls to getters directly rather than through a C helper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 129778 130644 130707
Blocks: 129588
  Show dependency treegraph
 
Reported: 2014-03-02 17:12 PST by Filip Pizlo
Modified: 2014-03-27 17:38 PDT (History)
15 users (show)

See Also:


Attachments
it begins (12.93 KB, patch)
2014-03-24 23:02 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (15.46 KB, patch)
2014-03-24 23:18 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles and instacrashes (30.54 KB, patch)
2014-03-25 11:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles and doesn't crash (33.72 KB, patch)
2014-03-25 12:03 PDT, Filip Pizlo
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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