Bug 208725 - JSON.stringify should call replacer on deleted properties
Summary: JSON.stringify should call replacer on deleted properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-06 11:33 PST by Alexey Shvayka
Modified: 2020-03-07 10:18 PST (History)
9 users (show)

See Also:


Attachments
Patch (6.85 KB, patch)
2020-03-06 13:41 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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?