Bug 208725

Summary: JSON.stringify should call replacer on deleted properties
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=205769
https://bugs.webkit.org/show_bug.cgi?id=208766
Attachments:
Description Flags
Patch none

Comment 1 Alexey Shvayka 2020-03-06 13:41:57 PST 
Created attachment 392766 [details]
Patch
Comment 2 Alexey Shvayka 2020-03-06 13:52:21 PST 
`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
Comment 3 Ross Kirsling 2020-03-06 15:52:28 PST 
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).
Comment 4 Alexey Shvayka 2020-03-06 16:21:44 PST 
(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`.
Comment 5 WebKit Commit Bot 2020-03-06 19:02:48 PST 
Comment on attachment 392766 [details]
Patch

Clearing flags on attachment: 392766

Committed r258049: <https://trac.webkit.org/changeset/258049>
Comment 6 WebKit Commit Bot 2020-03-06 19:02:49 PST 
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2020-03-06 19:03:14 PST 
<rdar://problem/60179986>
Comment 8 Ryan Haddad 2020-03-07 10:18:40 PST 
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?