Bug 160922

Summary: GetByIdWithThis/GetByValWithThis should have ValueProfiles so that they can predict their result types
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
profile of super-get-by-id-with-this-{mono,poly}morphic.js
none
run-jsc-benchmarks none

Description Filip Pizlo 2016-08-16 16:59:58 PDT 
...
Comment 1 JF Bastien 2016-09-01 10:58:08 PDT 
Created attachment 287635 [details]
patch

The patch should be ready to review. As follow-ups we could:
 - JIT both instead of just having a slow path (Saam mentioned that there was already a bug for this).
 - Add array profiling for GetByValWithThis.

Result of the benchmarks I'm adding:

$ ./Tools/Scripts/run-jsc-benchmarks old:./before/bin/jsc new:./current/bin/jsc --benchmarks "super-get-by-" --microbenchmarks --outer 400

                                                   old                       new                                        

super-get-by-id-with-this-monomorphic        29.3852+-0.2030     ^     27.1366+-0.1896        ^ definitely 1.0829x faster
super-get-by-id-with-this-polymorphic        27.0021+-0.2115     ^     25.6205+-0.1965        ^ definitely 1.0539x faster
super-get-by-val-with-this-monomorphic       29.8851+-0.1339     ^     28.9029+-0.1612        ^ definitely 1.0340x faster
super-get-by-val-with-this-polymorphic       27.6204+-0.1616     ?     27.8924+-0.2388        ?

<geometric>                                  28.4117+-0.0838     ^     27.3187+-0.0971        ^ definitely 1.0400x faster

Note in the results: why is monomorphy slower? It was slower before, and is still slower now so I think we can investigate separately. The difference seems to be that there were many more exits in mono than poly, even before my change (and yet more after my change, but things still got faster).
Comment 2 JF Bastien 2016-09-01 10:58:53 PDT 
Created attachment 287636 [details]
profile of super-get-by-id-with-this-{mono,poly}morphic.js
Comment 3 JF Bastien 2016-09-01 10:59:35 PDT 
Created attachment 287637 [details]
run-jsc-benchmarks
Comment 4 Keith Miller 2016-09-01 14:47:21 PDT 
Comment on attachment 287635 [details]
patch

r=me.
Comment 5 WebKit Commit Bot 2016-09-01 15:10:16 PDT 
Comment on attachment 287635 [details]
patch

Clearing flags on attachment: 287635

Committed r205321: <http://trac.webkit.org/changeset/205321>
Comment 6 WebKit Commit Bot 2016-09-01 15:10:21 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Saam Barati 2016-09-01 16:10:11 PDT 
Comment on attachment 287635 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4137
> +            Node* getByValWithThis = addToGraph(GetByValWithThis, OpInfo(prediction), base, thisValue, property);

This code is wrong, it needs to be the second OpInfo.