Skip to content

improve performance on error path in JSON.parse#90

Merged
mcollina merged 5 commits intomasterfrom
performance
Jan 10, 2023
Merged

improve performance on error path in JSON.parse#90
mcollina merged 5 commits intomasterfrom
performance

Conversation

@Uzlopak
Copy link
Copy Markdown
Contributor

@Uzlopak Uzlopak commented Jan 9, 2023 •
edited
Loading

I investigated the performance of secure-json-parse and I think I found a potential peformance optimization. But I am unsure how useful it is in regard of fastify.

I optimized few months ago secure-json-parse safeParse method, but I didnt apply it to fastify, because Errors are needed to give useful feedback. So we didnt have any benefit from the performance chances back then.

So I realized, that the stackTrace is one of the reasons, we have performance degredation. So in a naive attempt I propose this patch. I mean... it is not nice to set Error.stackTraceLimit to 0 and then back to 10 every time we parse JSON. Can there be some kind of race condition? idk.

But here I propose a solution, which improves the performance of secure-json-parse in case a malicious attacker sends invalid payloads.

before:

node benchmarks/valid.js
JSON.parse x 1,571,384 ops/sec ±0.24% (95 runs sampled)
JSON.parse proto x 1,048,214 ops/sec ±1.14% (88 runs sampled)
secure-json-parse parse x 1,430,655 ops/sec ±1.03% (91 runs sampled)
secure-json-parse parse proto x 1,589,677 ops/sec ±1.20% (93 runs sampled)
secure-json-parse safeParse x 1,333,056 ops/sec ±0.70% (87 runs sampled)
secure-json-parse safeParse proto x 930,279 ops/sec ±1.02% (93 runs sampled)
JSON.parse reviver x 495,446 ops/sec ±0.92% (92 runs sampled)
Fastest is secure-json-parse parse proto,JSON.parse

JSON.parse valid x 1,052,749 ops/sec ±1.26% (91 runs sampled)
JSON.parse error x 158,899 ops/sec ±1.96% (81 runs sampled)
secure-json-parse parse x 146,439 ops/sec ±1.34% (89 runs sampled)
secure-json-parse safeParse x 146,369 ops/sec ±1.02% (83 runs sampled)
reviver x 172,568 ops/sec ±1.45% (92 runs sampled)
Fastest is JSON.parse valid

after:

node benchmarks/valid.js
JSON.parse x 1,778,868 ops/sec ±0.21% (92 runs sampled)
JSON.parse proto x 1,155,303 ops/sec ±0.11% (90 runs sampled)
secure-json-parse parse x 1,417,567 ops/sec ±0.92% (89 runs sampled)
secure-json-parse parse proto x 1,681,807 ops/sec ±0.14% (98 runs sampled)
secure-json-parse safeParse x 1,352,484 ops/sec ±0.11% (98 runs sampled)
secure-json-parse safeParse proto x 907,907 ops/sec ±0.08% (97 runs sampled)
JSON.parse reviver x 493,532 ops/sec ±0.09% (99 runs sampled)
Fastest is JSON.parse

 node benchmarks/throw.js
JSON.parse valid x 1,162,803 ops/sec ±0.39% (92 runs sampled)
JSON.parse error x 180,638 ops/sec ±0.30% (91 runs sampled)
secure-json-parse parse x 285,829 ops/sec ±0.17% (95 runs sampled)
secure-json-parse safeParse x 385,064 ops/sec ±0.53% (97 runs sampled)
reviver x 152,137 ops/sec ±0.20% (88 runs sampled)
Fastest is JSON.parse valid

Checklist

@Uzlopak Uzlopak changed the title improve performance on SyntaxErrors improve performance on error path in JSON.parse Jan 9, 2023
@Uzlopak Uzlopak requested a review from jsumners January 9, 2023 15:56
Eomm
Eomm reviewed Jan 9, 2023
View reviewed changes
@Uzlopak
Copy link
Copy Markdown
Contributor Author

Uzlopak commented Jan 9, 2023

updated benchmarks

@Uzlopak Uzlopak requested review from Eomm and RafaelGSS January 10, 2023 07:54
@Uzlopak
Copy link
Copy Markdown
Contributor Author

Uzlopak commented Jan 10, 2023

So what do you think?

We use the parse method in fastify and this PR improves the performance of the error path from about 140k ops/s to about 280k ops/s. So we double the performance for this actually risky path.

The only thing which makes me wonder is, if there is a potential race condition by setting globally Error.stackTraceLimit. like you call the sync function parse in an async function and while node is running JSON.parse, another process depending on the stack trace just gets the empty stack trace. Is this possible?

mcollina
mcollina approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I think the benefit outweighs the danger.

jsumners
jsumners reviewed Jan 10, 2023
View reviewed changes
@mcollina mcollina merged commit 245a0dc into master Jan 10, 2023
@mcollina mcollina deleted the performance branch January 10, 2023 14:26
@gr2m gr2m mentioned this pull request Apr 7, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jsumners jsumners jsumners approved these changes

@climba03003 climba03003 Awaiting requested review from climba03003

@Eomm Eomm Awaiting requested review from Eomm

@RafaelGSS RafaelGSS Awaiting requested review from RafaelGSS

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Uzlopak @mcollina @jsumners @Eomm @RafaelGSS