Skip to content

fix "isLong is not defined" error #73

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

Merged
merged 4 commits into from
Feb 8, 2018
Merged

fix "isLong is not defined" error #73

merged 4 commits into from
Feb 8, 2018

Conversation

chrisguttandin
Copy link
Contributor

After updating to version 1, I encountered an "isLong is not defined" error. I figured that was because I am not using the long package and it isn't installed at all in my package. Here is a little example to illustrate the problem.

const fastJson = require('fast-json-stringify');

fastJson({
    "properties": {
        "x": {
            "type": "integer"
        }
    },
    "$schema": "http://json-schema.org/draft-07/schema#",
    "type": "object"
})({ x: 2 });

Running that code will trigger the error.

I fixed the problem by extracting the checks for isLong from the generated code. I'm not entirely sure if that's the way it should be done, but I can imagine that it will also bring some little speed improvements as it does not insert if statements anymore which are going to fail anyway.

Please let me know if you want me to make any changes.

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

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Looks good, can you run some benchmarks?

@allevo
Copy link
Member

allevo commented Feb 7, 2018

Why aren't there failing tests?

@mcollina
Copy link
Member

mcollina commented Feb 7, 2018

@chrisguttandin
Copy link
Contributor Author

@mcollina I basically copied the tests for long values and changed all the values to be of type integer. Thanks for pointing me to proxyquire. Let me know what you think.

@mcollina
Copy link
Member

mcollina commented Feb 7, 2018

@chrisguttandin can you run the benchmarks before/after? I think we can land this as I don't expect any regressions.

@chrisguttandin
Copy link
Contributor Author

@mcollina No problem. These are the results:

before:

JSON.stringify array x 2,540 ops/sec ±2.77% (81 runs sampled)
fast-json-stringify array x 2,940 ops/sec ±3.54% (79 runs sampled)
fast-json-stringify-uglified array x 1,804 ops/sec ±18.49% (53 runs sampled)
JSON.stringify long string x 8,307 ops/sec ±5.03% (73 runs sampled)
fast-json-stringify long string x 8,966 ops/sec ±5.22% (78 runs sampled)
fast-json-stringify-uglified long string x 7,986 ops/sec ±3.71% (73 runs sampled)
JSON.stringify short string x 3,161,861 ops/sec ±3.63% (76 runs sampled)
fast-json-stringify short string x 6,108,401 ops/sec ±3.68% (78 runs sampled)
fast-json-stringify-uglified short string x 5,542,691 ops/sec ±10.71% (71 runs sampled)
JSON.stringify obj x 1,031,917 ops/sec ±9.72% (67 runs sampled)
fast-json-stringify obj x 2,186,183 ops/sec ±9.53% (67 runs sampled)
fast-json-stringify-uglified obj x 1,948,209 ops/sec ±7.23% (63 runs sampled)

after:

JSON.stringify array x 2,572 ops/sec ±2.63% (82 runs sampled)
fast-json-stringify array x 2,875 ops/sec ±4.54% (74 runs sampled)
fast-json-stringify-uglified array x 1,720 ops/sec ±17.63% (46 runs sampled)
JSON.stringify long string x 8,943 ops/sec ±9.15% (77 runs sampled)
fast-json-stringify long string x 9,774 ops/sec ±2.93% (82 runs sampled)
fast-json-stringify-uglified long string x 8,932 ops/sec ±4.24% (77 runs sampled)
JSON.stringify short string x 3,082,880 ops/sec ±4.16% (74 runs sampled)
fast-json-stringify short string x 7,078,220 ops/sec ±1.25% (86 runs sampled)
fast-json-stringify-uglified short string x 5,421,092 ops/sec ±15.64% (67 runs sampled)
JSON.stringify obj x 624,898 ops/sec ±19.23% (49 runs sampled)
fast-json-stringify obj x 2,025,581 ops/sec ±18.49% (61 runs sampled)
fast-json-stringify-uglified obj x 491,831 ops/sec ±31.24% (37 runs sampled)

@mcollina
Copy link
Member

mcollina commented Feb 7, 2018

Can you run them again? The performance of JSON.stringify is significantly different between the two runs, maybe you had something else running on your machine.

@chrisguttandin
Copy link
Contributor Author

I ran the "JSON.stringify obj" test a couple more times:

before:

JSON.stringify obj x 1,130,135 ops/sec ±8.23% (74 runs sampled)
JSON.stringify obj x 788,081 ops/sec ±3.88% (68 runs sampled)
JSON.stringify obj x 1,284,750 ops/sec ±1.23% (84 runs sampled)
JSON.stringify obj x 1,427,471 ops/sec ±2.01% (86 runs sampled)

after:

JSON.stringify obj x 1,153,374 ops/sec ±3.33% (78 runs sampled)
JSON.stringify obj x 879,321 ops/sec ±3.54% (61 runs sampled)
JSON.stringify obj x 1,267,546 ops/sec ±2.28% (81 runs sampled)
JSON.stringify obj x 1,381,669 ops/sec ±2.10% (85 runs sampled)

@mcollina
Copy link
Member

mcollina commented Feb 8, 2018

This PR

JSON.stringify array x 3,184 ops/sec ±2.70% (85 runs sampled)
fast-json-stringify array x 6,199 ops/sec ±2.86% (83 runs sampled)
fast-json-stringify-uglified array x 6,208 ops/sec ±2.04% (84 runs sampled)
JSON.stringify long string x 9,338 ops/sec ±1.96% (85 runs sampled)
fast-json-stringify long string x 9,446 ops/sec ±2.32% (86 runs sampled)
fast-json-stringify-uglified long string x 9,501 ops/sec ±1.85% (87 runs sampled)
JSON.stringify short string x 3,933,694 ops/sec ±1.84% (84 runs sampled)
fast-json-stringify short string x 21,388,265 ops/sec ±2.28% (81 runs sampled)
fast-json-stringify-uglified short string x 21,356,291 ops/sec ±2.63% (82 runs sampled)
JSON.stringify obj x 1,627,522 ops/sec ±2.00% (87 runs sampled)
fast-json-stringify obj x 5,328,505 ops/sec ±2.21% (87 runs sampled)
fast-json-stringify-uglified obj x 5,260,963 ops/sec ±2.25% (85 runs sampled)

master

JSON.stringify array x 3,106 ops/sec ±2.62% (82 runs sampled)
fast-json-stringify array x 6,152 ops/sec ±2.56% (81 runs sampled)
fast-json-stringify-uglified array x 6,287 ops/sec ±2.40% (85 runs sampled)
JSON.stringify long string x 9,359 ops/sec ±2.34% (84 runs sampled)
fast-json-stringify long string x 9,722 ops/sec ±2.14% (88 runs sampled)
fast-json-stringify-uglified long string x 9,502 ops/sec ±2.27% (86 runs sampled)
JSON.stringify short string x 4,055,616 ops/sec ±1.98% (87 runs sampled)
fast-json-stringify short string x 21,488,267 ops/sec ±1.58% (86 runs sampled)
fast-json-stringify-uglified short string x 20,683,277 ops/sec ±2.89% (83 runs sampled)
JSON.stringify obj x 1,626,478 ops/sec ±1.63% (88 runs sampled)
fast-json-stringify obj x 5,302,727 ops/sec ±3.21% (86 runs sampled)
fast-json-stringify-uglified obj x 5,566,143 ops/sec ±1.72% (87 runs sampled)

Copy link
Member

@delvedor delvedor 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 ebd32f8 into fastify:master Feb 8, 2018
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.

4 participants