Bug 208725 – JSON.stringify should call replacer on deleted properties

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

[Alexey Shvayka]

ECMA262: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a)
Test262: https://test262.report/browse/built-ins/JSON/stringify/replacer-function-object-deleted-property.js

[Alexey Shvayka]

Created attachment 392766 [details]
Patch

[Alexey Shvayka]

`toJSONImpl` inlining, cold runs, --outer 4:

                                            TipOfTree              patch

json-stringify-many-objects              554.1745+-7.1931     555.3961+-5.6955
json-stringify-many-objects-to-json      498.5819+-6.1926     495.1950+-6.4387

<geometric>                              525.6438+-6.6741     524.4324+-6.0557

[Ross Kirsling]

Comment on attachment 392766 [details]
Patch

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

This seems reasonable, particularly given the discussion about `get` in the previous patch.

I'm not used to practices surrounding microbenchmarks, but they seem reasonable to me based on existing ones.

> Source/JavaScriptCore/ChangeLog:15
> +        [1]: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a)

Is this sufficient/correct as a spec link? It seems like all three cases are removing early outs, but this just covers the object case (though I guess that's the one case with the visible conformance failure).

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #3)
> I'm not used to practices surrounding microbenchmarks, but they seem reasonable to me based on existing ones.

Given the possibility that `toJSONImpl` was extracted for performance reasons, these microbenchmarks are designed to spot any regression in "toJSON" lookup.

> Is this sufficient/correct as a spec link? It seems like all three cases are
> removing early outs, but this just covers the object case (though I guess
> that's the one case with the visible conformance failure).

Right, object case is the only one that was not spec-conformant because of

```
if (!hasProperty)
    return true;
```

The other two were 100% equivalent to `get`.

[WebKit Commit Bot]

Comment on attachment 392766 [details]
Patch

Clearing flags on attachment: 392766

Committed r258049: <https://trac.webkit.org/changeset/258049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60179986>

[Ryan Haddad]

This change cappears to have caused assertion failures on the Debug JSC bot:
https://bugs.webkit.org/show_bug.cgi?id=208766

Can this be addressed quickly?