Bug 160922 – GetByIdWithThis/GetByValWithThis should have ValueProfiles so that they can predict their result types

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

[Filip Pizlo]

...

[JF Bastien]

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

[JF Bastien]

Created attachment 287636 [details]
profile of super-get-by-id-with-this-{mono,poly}morphic.js

[JF Bastien]

Created attachment 287637 [details]
run-jsc-benchmarks

[Keith Miller]

Comment on attachment 287635 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 287635 [details]
patch

Clearing flags on attachment: 287635

Committed r205321: <http://trac.webkit.org/changeset/205321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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.