improve performance on error path in JSON.parse

[Uzlopak]

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

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

updated benchmarks

[Uzlopak]

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]

LGTM

I think the benefit outweighs the danger.