Thank you for these performance suggestions! Would you mind rebasing as we've made some CI changes and I'd like to see the results of this PR in CI. Thanks!

Done, I've rebased on main.

edit: And it's failing due to coverage. How can I see which lines aren't covered?

To view coverage, run npm run coverage. This will create a file at coverage/lcov-report/index.html where you can review coverage in a browser.

Build passing & all good on my side, this can be merged if you're satisfied with it.

Linting complete.

edit: Oh wait no missed the 4 digits.
edit: Ok now we're good.

Awesome! I'm going to sit on this PR for a day just to make sure I'm not missing anything and vet the code a bit more. I really appreciate the code improvements and well-written description.

Ping @jordanbtucker: is there still interest for this PR? If not feel free to close.

I should also note, for completeness' sake, that if someone is really after performance, then using a pure javascript implementation of JSON5 won't cut it. The native JSON implementation in node/V8 is 20-40x faster in the same benchmarks used above. But saving cycles is always nice.

If you're not able to make those changes, I don't mind merging this and making them myself.

Also, I think I might rebase this against the v3 branch instead of main.

Everything is good except for the the hex numbers. It would be great if you could refactor them as constants instead of using block comments to identify them.

Something like this?

const C_0 = 0x30
const C_1 = 0x31
// ...

Yeah, that works. Maybe use CHAR_0 instead though, just to make it more clear that these represent characters?

I've been trying to find time for this but I've been super busy with work. If you're ok with merging as-is I think it's going to be faster. I've rebased on master, do you also need it rebased on v3?

any update here? Imho those improvements would be quite useful.

Ping @jordanbtucker, if you can list the minimum required changes to merge this, I might be able to complete it soon.

Somewhat criminal this branch was never merged.

Open-source maintainers are free to merge or not, performance might not be a priority for this project. Merging this PR also means the maintainer(s) would need to support these changes, which do decrease readability a bit :|

Note that from what I remember while benchmarking this PR, even with these changes the native JSON.parse is something like 20x faster so anyone using this JSON5 parser should be using it because readability is more important for their use-case than performance. If you need to pass a lot of data around, use standard JSON. I would find it nice to add a note on the readme about the performance characteristics of the project though, to avoid people using it in cases where it wouldn't be a good fit.

@romgrk, while this PR sits here I've found a home for a somewhat modified version of your suggested changes in my own JSON5 fork. Thanks for the ideas!

+1 vote to merge this PR. as I just clone this PR's branch and do the benchmark myself and found it indeed improve the performance by x2 simply use enum integer instead of enum string (I lied to make my vote more creditable :v)

I added some benchmarking to my own spin-off project. The benchmarking measures the performance of the built-in JSON.parse() function, and how the current JSON5, my old JSON-Z without the above enhancements (plus one other), and my new JSON-Z perform.

--------- 40 MB sample benchmarks (ranked) ----------
1. JSON      x 11.14 ops/sec ±5.31% (33 runs sampled)
2. JSONZ     x  0.99 ops/sec ±3.17% (7 runs sampled)
3. JSON5     x  0.48 ops/sec ±5.21% (6 runs sampled)
4. Old JSONZ x  0.35 ops/sec ±7.57% (5 runs sampled)
-----------------------------------------------------

--- 60 MB long string sample benchmarks (ranked) ----
1. JSON      x 7.85 ops/sec ±10.93% (24 runs sampled)
2. JSONZ     x 0.75 ops/sec ±2.01% (6 runs sampled)
3. JSON5     x 0.26 ops/sec ±7.38% (5 runs sampled)
4. Old JSONZ x 0.18 ops/sec ±0.61% (5 runs sampled)
-----------------------------------------------------

The test material has to be standard JSON, of course, or JSON.parse() couldn't be included in the benchmarks.

The first sample is a huge sample file I found, naturally enough, googling for "large JSON sample". There might well be better samples out there, but this is probably decent. (I'll may try to find another example that exercises number, boolean, and null parsing more.)

The second sample is simply the first sample with a 20MB-long single string tacked on, related to another speed improvement from this PR: #233

Clearly nothing else comes close to JSON.parse() for speed, but at least these enhancements narrow the gap significantly.