Skip to content
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

Add comment support #91

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add comment support #91

wants to merge 1 commit into from

Conversation

1aam2am1
Copy link

#90 Add basic comment support

Copy link

codspeed-hq bot commented May 11, 2024

CodSpeed Performance Report

Merging #91 will degrade performances by 24.96%

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

Summary

❌ 12 regressions
✅ 61 untouched benchmarks

⚠️ 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%

@1aam2am1 1aam2am1 force-pushed the main branch 2 times, most recently from 45d4565 to 6110cb2 Compare May 11, 2024 18:29
Copy link

codecov bot commented May 11, 2024

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 ⚠️

📢 Thoughts on this report? Let us know!

@davidhewitt
Copy link
Collaborator

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

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.

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.

None yet

2 participants