Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update JSON response stringify for better performance #3014

Merged
merged 4 commits into from Feb 4, 2016

Conversation

@feego
Copy link
Contributor

@feego feego commented Jan 6, 2016

Hi, I've been doing some profiling to a REST API I've been working on and noticed that, for large JSON responses, a lot of time was being spent stringifying the actual responses. After some digging, I found out that the performance of the native V8 JSON.stringify degrades very quickly when the method is called with more than 1 argument (the Javascript object to stringify itself), even if the other arguments are null or undefined.

I've done some simple but yet conclusive tests with JSON.stringify and got some results to share:

// Copy pasted data into variable fathers.
var fathers = ....;

// Test 1.
var startTime = Date.now();
JSON.stringify(fathers, null, null);
console.log(Date.now() - startTime);
// Prints 215.

// Test 2.
startTime = Date.now();
JSON.stringify(fathers);
console.log(Date.now() - startTime);
// Prints 20.

Hapi is currently stringifying all JSON response objects calling JSON.stringify always with all the 3 arguments, even if the last 2 are null. I updated the JSON.stringify call in Hapi to use this last 2 arguments only when they have non falsy values.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 6, 2016

Interesting. I don't mind the optimization but if should be implemented using an if statement, not apply. That should also fix the exception your code is throwing.

Loading

@feego
Copy link
Contributor Author

@feego feego commented Jan 6, 2016

Thanks, it's done.

Loading

@leesei
Copy link

@leesei leesei commented Feb 4, 2016

I've made a jsPerf for this finding:
JSON.stringify() w/ and w/o param · jsPerf

I think it's a v8 bug, opened issue here.

Loading

@leesei
Copy link

@leesei leesei commented Feb 4, 2016

It is confirmed by the v8 team that this is the expected behavior.
This v8 specific optimization LGTM.

Loading

@hueniverse hueniverse self-assigned this Feb 4, 2016
@hueniverse hueniverse added this to the 13.0.1 milestone Feb 4, 2016
hueniverse added a commit that referenced this issue Feb 4, 2016
Update JSON response stringify for better performance
@hueniverse hueniverse merged commit 05bfc3e into hapijs:master Feb 4, 2016
1 check passed
Loading
kisg pushed a commit to paul99/v8mips that referenced this issue Feb 20, 2016
Most libraries use `JSON.stringify` with all three arguments [1] to allow for
configuration, even if `replacer` and `space` are falsey, causing the
optimized native stringifying to be missed. This commit allows for the common
case where `replacer` and `space` are not used to be fast.

[1]: hapijs/hapi#3014

BUG=v8:4730
LOG=N

R=yangguo@chromium.org

Review URL: https://codereview.chromium.org/1710933002

Cr-Commit-Position: refs/heads/master@{#34174}
@mcollina
Copy link

@mcollina mcollina commented Apr 8, 2016

Passing here by change: you should use http://npm.im/fast-safe-stringify by @davidmarkclements. It "fixes" circular references and it is incredibly fast.

Loading

@davidmarkclements
Copy link

@davidmarkclements davidmarkclements commented Apr 8, 2016

Just to clarify, it's not as fast as using JSON.stringify as-is, but as far as I know it's the fastest "safe stringifer" - particularly for large objects

fast-safe-stringify is 2x faster for small objects, and 6x faster for large objects than json-stringify-safe

If you have no concerns around circular references then to my knowledge, directly using JSON.stringify is as fast as you can get atm.

But yeah I noticed the same degradation with additional params, which led to the technique used in fast-safe-stringify

Loading

@mcollina
Copy link

@mcollina mcollina commented Apr 8, 2016

Also there is a try catch, which is not needed with fast-safe-stringify
Il giorno ven 8 apr 2016 alle 19:28 David Mark Clements <
notifications@github.com> ha scritto:

Just to clarify, it's not as fast as using JSON.stringify as-is, but as
far as I know it's the fastest "safe stringifer" - particularly for large
objects

fast-safe-stringify is 2x faster for small objects, and 6x faster for
large objects than json-stringify-safe

If you have no concerns around circular reference then to my knowledge,
directly using JSON.stringify is as fast as you can get atm.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#3014 (comment)

Loading

@devinivy
Copy link
Member

@devinivy devinivy commented Apr 8, 2016

@mcollina That's actually a pretty good point– the try/catch deoptimizes that internal _streamify() function. If someone has the time, I'd be interested to see if breaking it out into a minimal function makes a difference.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 9, 2016

Thanks but not going to add external deps.

Loading

@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

Loading

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants