Optimize JSON.stringify performance in V8.

[bnjmnt4n]

Older versions of V8 have a bug 1 where using empty replacer or
space arguments decreases performance.

[tunniclm]

Nice 👍

[dougwilson]

Very interesting, didn't know there was a separate code path based on argument count. I think this is a good change, and unless there is an objection, slated to 4.14

[Fishrock123]

lgtm

[bnjmnt4n]

Any updates?

[DavidTPate]

LGTM

[dougwilson]

Hi @demoneaux, we are planning to add this into the 4.14 release. We are working on documenting our release process and will be merging your pull request into an appropriate branch soon.

[ronkorving]

Just keep in mind that this is automatically resolved in upcoming Node versions with the inclusion of newer V8 engine. So while it's an optimization today (and by all means, go for it), it will be noise in the future. You may want to add a code comment stating from which V8 version this workaround is no longer needed (and I forgot which version... not very helpful am I?).

[mishelashala]

LGTM

[RWOverdijk]

2 months in. Still working on the documentation? Just curious.

[ronkorving]

Node 6 should not have this performance issue btw.

[dougwilson]

This change looks good to me. It would be neat if we could get a comment in the code to understand when we can eventually remove this conditional (like what v8 or even Node.js version it's fixed in), but that would not bar getting this landed in the 4.14 release :) The 4.14 branch was created, and we'll get a 4.14 release pull request going from there with this change.

@expressjs/express-tc anyone have thoughts on when to close pull requests that are not merged on master? Historically I have debated between the following two possibilities, but every time I decide on one, it seems a user thinks it should work the opposite way :)

1. Keep the pull request open even after merging to a release branch and just letting GitHub automatically close the pull request when it's finally merged back into master.
2. Manually close the pull request after merging into a release branch.

Typically I have heard the following arguments against each of those:

1. The typical argument against this one is that "when will this PR be closed?" and general impatience with a lingering PR.
2. The typical argument against this one I see is usually only when someone makes a duplicate PR/issue for an already resolved one. It typically comes down to "I didn't check any other branches/or the release PR", but there may be other ways to account for this, like clearer guidelines and pointers of where people can check for these things.

[hacksparrow]

@demoneaux, which older versions of V8/Node are affected?

@dougwilson I am in favor of option 1, with a comment that it has been merged on a branch, and will eventually make it to master.

[ronkorving]

I believe Node pre-6 is affected by this issue.