Bug 205769 – JSON.parse should lookup prototype chains during revival

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

[Alexey Shvayka]

ECMA262: https://tc39.es/ecma262/#sec-internalizejsonproperty (step 1)
Test262:
  https://test262.report/browse/built-ins/JSON/parse/reviver-array-get-prop-from-prototype.js
  https://test262.report/browse/built-ins/JSON/parse/reviver-object-get-prop-from-prototype.js

Currently, JSC gets only own properties. ChakraCore fails array test, yet passes the object test.
Related steps of InternalizeJSONProperty were not normatively changed since ES5.

[Alexey Shvayka]

Created attachment 386770 [details]
Patch

[Darin Adler]

Comment on attachment 386770 [details]
Patch

I wanted to review this, and the change seems clean and straightforward, but I’m not enough expert on what optimization might be lost by using the get functions in the most ordinary way. My guess is that there’s no downside to this change, but I don’t feel that I am completely qualified.

Someone with more recent understanding of the runtime could weigh in on that point.

[Saam Barati]

Comment on attachment 386770 [details]
Patch

r=me

[Saam Barati]

(In reply to Darin Adler from comment #2)
> Comment on attachment 386770 [details]
> Patch
> 
> I wanted to review this, and the change seems clean and straightforward, but
> I’m not enough expert on what optimization might be lost by using the get
> functions in the most ordinary way. My guess is that there’s no downside to
> this change, but I don’t feel that I am completely qualified.
> 
> Someone with more recent understanding of the runtime could weigh in on that
> point.

It should be the essentially the same speed based on my reading of the code. Where it’s not is where we’re actually doing more work in order to follow the spec, when we traverse up the prototype chain searching for the property.

[Alexey Shvayka]

Comment on attachment 386770 [details]
Patch

Thank you for reviews, folks. I've paused the commit queue to run some microbenchmarks: no measurable regression.

Cold runs, --outer 8:

                                   TipOfTree              patch

json-parse-array-reviver       193.4741+-1.9814      196.1498+-1.9866
json-parse-object-reviver      218.0244+-1.2942      217.8842+-1.4113

<geometric>                    205.3828+-1.6014      206.7316+-1.6745

[WebKit Commit Bot]

Comment on attachment 386770 [details]
Patch

Clearing flags on attachment: 386770

Committed r254757: <https://trac.webkit.org/changeset/254757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58691323>