Bug 160922 - GetByIdWithThis/GetByValWithThis should have ValueProfiles so that they can predict their result types
Summary: GetByIdWithThis/GetByValWithThis should have ValueProfiles so that they can p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-16 16:59 PDT by Filip Pizlo
Modified: 2016-09-01 16:10 PDT (History)
5 users (show)

See Also:


Attachments
patch (18.11 KB, patch)
2016-09-01 10:58 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
profile of super-get-by-id-with-this-{mono,poly}morphic.js (16.12 KB, application/octet-stream)
2016-09-01 10:58 PDT, JF Bastien
no flags Details
run-jsc-benchmarks (80.91 KB, application/octet-stream)
2016-09-01 10:59 PDT, JF Bastien
no flags Details

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