pydantic / jiter

Fast iterable JSON parser.
https://crates.io/crates/jiter
MIT License
182 stars 11 forks source link

Add comment support #91

Closed 1aam2am1 closed 4 weeks ago

1aam2am1 commented 3 months ago

90 Add basic comment support

codspeed-hq[bot] commented 3 months ago

CodSpeed Performance Report

Merging #91 will degrade performances by 24.96%

Comparing 1aam2am1:main (830309a) with main (75699eb)

Summary

❌ 12 regressions ✅ 61 untouched benchmarks

:warning: Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 1aam2am1:main Change
bigints_array_jiter_skip 500.4 µs 556.9 µs -10.16%
medium_response_jiter_skip 73.5 µs 83.7 µs -12.19%
pass1_jiter_skip 53.2 µs 60.6 µs -12.17%
pass2_jiter_iter 19.4 µs 21.6 µs -10.28%
pass2_jiter_skip 5.5 µs 7.4 µs -24.96%
short_numbers_jiter_skip 332.7 µs 374.5 µs -11.17%
string_array_jiter_iter 43.3 µs 50.9 µs -14.82%
string_array_jiter_skip 37.2 µs 42.9 µs -13.29%
true_array_jiter_iter 24 µs 27.4 µs -12.5%
true_array_jiter_skip 22.9 µs 27.6 µs -17.05%
true_object_jiter_skip 56.2 µs 66.7 µs -15.67%
python_parse_true_array 49.8 µs 56 µs -11.11%
codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes are missing coverage. Please review. Files Patch % Lines
crates/jiter/src/parse.rs 0.00% 21 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!

davidhewitt commented 3 months ago

The use case of supporting comments is 👍 from me, but I would like us to find a way to do this without costing performance.

I suspect that the introduction of a large match arm inside the eat_whitespace function has led to the performance regression.

The fuzz jobs are failing because serde doesn't support comments.

The most practical way to solve both of these may be to make comment support optional and find a way to refactor to avoid the perf hit when unused.

1aam2am1 commented 3 months ago

Hi. This is great idea, to support it as a option, where you could toggle it. For example from pydantic python.

But as I don't know rust very much and don't know exactly how this project here is done, I can't write a updated code. I think we would need some template programming (If something like this exists in rust). And simply select engine with comments or without depending on option. So that we don't have extra branch there. As any if added there would have speed penalty.

davidhewitt commented 3 months ago

Yep, Rust has generics which are the closest equivalent it has to C++ templates.

This isn't high priority for me to pick up right now, but anyone with interest could take this on (or maybe I can add in the future).

samuelcolvin commented 4 weeks ago

closing until someone wants to take this on.