Bug 160922

Summary:

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

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, keith_miller, mark.lam, msaboff, saam

Priority:

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

Filip Pizlo

Reported 2016-08-16 16:59:58 PDT

...

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
View All Add attachment proposed patch, testcase, etc.

JF Bastien

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

JF Bastien

Comment 2 2016-09-01 10:58:53 PDT

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

JF Bastien

Comment 3 2016-09-01 10:59:35 PDT

Created attachment 287637 [details] run-jsc-benchmarks

Keith Miller

Comment 4 2016-09-01 14:47:21 PDT

Comment on attachment 287635 [details] patch r=me.

WebKit Commit Bot

Comment 5 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>

WebKit Commit Bot

Comment 6 2016-09-01 15:10:21 PDT

All reviewed patches have been landed. Closing bug.

Saam Barati

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

Note You need to log in before you can comment on or make changes to this bug.