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+

Filip Pizlo
Reported 2014-03-02 17:12:26 PST
...
Attachments
it begins (12.93 KB, patch)
2014-03-24 23:02 PDT, Filip Pizlo
no flags
more (15.46 KB, patch)
2014-03-24 23:18 PDT, Filip Pizlo
no flags
it compiles and instacrashes (30.54 KB, patch)
2014-03-25 11:38 PDT, Filip Pizlo
no flags
it compiles and doesn't crash (33.72 KB, patch)
2014-03-25 12:03 PDT, Filip Pizlo
mhahnenberg: review+
Filip Pizlo
Comment 1 2014-03-24 23:02:40 PDT
Created attachment 227722 [details] it begins
Filip Pizlo
Comment 2 2014-03-24 23:18:20 PDT
Filip Pizlo
Comment 3 2014-03-25 11:38:01 PDT
Created attachment 227776 [details] it compiles and instacrashes
Filip Pizlo
Comment 4 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.
WebKit Commit Bot
Comment 5 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.
Mark Hahnenberg
Comment 6 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
Filip Pizlo
Comment 7 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
Filip Pizlo
Comment 8 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.
Filip Pizlo
Comment 9 2014-03-25 16:29:39 PDT
Filip Pizlo
Comment 11 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.
Oliver Hunt
Comment 12 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
Note You need to log in before you can comment on or make changes to this bug.