Skip to content

Conversation

Eomm
Copy link
Member

@Eomm Eomm commented Feb 3, 2019

Hi, this PR fixes #136

I run the bench.js three times with Node 10.11.
I don't expect any improvement or defect on overall performance because these changes should be executed only when a $ref = '#customId' is found.

master:

FJS creation x 5,465 ops/sec ±5.09% (84 runs sampled)
JSON.stringify array x 2,375 ops/sec ±1.04% (92 runs sampled)
fast-json-stringify array x 5,698 ops/sec ±2.89% (90 runs sampled)
fast-json-stringify-uglified array x 5,255 ops/sec ±3.61% (84 runs sampled)
JSON.stringify long string x 9,292 ops/sec ±0.73% (91 runs sampled)
fast-json-stringify long string x 9,155 ops/sec ±2.34% (92 runs sampled)
fast-json-stringify-uglified long string x 9,315 ops/sec ±0.30% (95 runs sampled)
JSON.stringify short string x 3,425,224 ops/sec ±6.24% (81 runs sampled)
fast-json-stringify short string x 29,926,884 ops/sec ±1.38% (88 runs sampled)
fast-json-stringify-uglified short string x 29,070,119 ops/sec ±1.53% (87 runs sampled)
JSON.stringify obj x 1,304,634 ops/sec ±1.25% (92 runs sampled)
fast-json-stringify obj x 5,742,953 ops/sec ±1.12% (89 runs sampled)
fast-json-stringify-uglified obj x 5,716,443 ops/sec ±1.03% (91 runs sampled)

this branch:

FJS creation x 5,498 ops/sec ±4.91% (84 runs sampled)
JSON.stringify array x 2,217 ops/sec ±4.27% (86 runs sampled)
fast-json-stringify array x 5,506 ops/sec ±4.50% (85 runs sampled)
fast-json-stringify-uglified array x 5,455 ops/sec ±4.81% (83 runs sampled)
JSON.stringify long string x 9,002 ops/sec ±2.77% (89 runs sampled)
fast-json-stringify long string x 9,312 ops/sec ±0.51% (94 runs sampled)
fast-json-stringify-uglified long string x 9,325 ops/sec ±0.34% (92 runs sampled)
JSON.stringify short string x 3,898,407 ops/sec ±0.98% (92 runs sampled)
fast-json-stringify short string x 28,256,237 ops/sec ±4.10% (82 runs sampled)
fast-json-stringify-uglified short string x 29,507,831 ops/sec ±1.74% (86 runs sampled)
JSON.stringify obj x 1,201,048 ops/sec ±7.02% (82 runs sampled)
fast-json-stringify obj x 5,713,316 ops/sec ±1.04% (88 runs sampled)
fast-json-stringify-uglified obj x 5,601,133 ops/sec ±1.63% (89 runs sampled)

@mcollina
Copy link
Member

mcollina commented Feb 4, 2019

Would this make it compatitble with fluent-schema?

index.js Outdated
code += `['${walk[i]}']`
var wl = walk.length
if (wl === 1) {
return searchForId(schema, `#${ref[1]}`)
Copy link
Member

Choose a reason for hiding this comment

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

Can you put in a couple of comments explaining the if condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, let me know if I should add more details

@Eomm
Copy link
Member Author

Eomm commented Feb 4, 2019

Would this make it compatitble with fluent-schema?

Yes, generally with this fix, the schema.response.200 settings in fastify will accept the $ref = '#customId attribute without failing (already tried to run this fix in fastify and works, will send a PR with this specific test 👍)

Copy link
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

@mcollina mcollina merged commit e3df63a into fastify:master Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maximum call stack size exceeded when $ref plain name fragment
2 participants