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

remove SmallVec and LazyIndexMap from json value #184

Merged
merged 6 commits into from
Feb 13, 2025
Merged

Conversation

davidhewitt
Copy link
Collaborator

@davidhewitt davidhewitt commented Jan 23, 2025

This simplifies the jiter public API (particularly on JsonValue) by simplifying the JsonObject and JsonArray types to just use Arc<Vec<...>> rather than smallvec in the array case and LazyIndexMap in the object case. This hasn't affected the way that we parse or the algorithmic complexity, it's a pure simplification.

At the same time we completely remove LazyIndexMap which attempted to be a thread-safe data structure which lazily constructed a mapping for iteration to guarantee uniqueness. I'm reasonably sure it became a "jack-of all trades, master of none" situation and there are bugs in it, maybe also performance costs.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.36%. Comparing base (4cefa8b) to head (9fabe08).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
- Coverage   88.71%   88.36%   -0.36%     
==========================================
  Files          13       12       -1     
  Lines        2207     2106     -101     
  Branches     2207     2106     -101     
==========================================
- Hits         1958     1861      -97     
+ Misses        153      151       -2     
+ Partials       96       94       -2     
Files with missing lines Coverage Δ
crates/jiter/src/lib.rs 93.33% <ø> (ø)
crates/jiter/src/value.rs 81.02% <100.00%> (-0.09%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cefa8b...9fabe08. Read the comment docs.

Copy link

codspeed-hq bot commented Jan 23, 2025

CodSpeed Performance Report

Merging #184 will improve performances by 11.14%

Comparing dh/simpler-value (9fabe08) with main (11ef20f)

Summary

⚡ 1 improvements
✅ 69 untouched benchmarks
⁉️ 3 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⁉️ lazy_map_lookup_1_10 9.2 µs N/A N/A
⁉️ lazy_map_lookup_2_20 25.8 µs N/A N/A
⁉️ lazy_map_lookup_3_50 52.4 µs N/A N/A
medium_response_jiter_value 41.8 µs 37.6 µs +11.14%

@davidhewitt davidhewitt marked this pull request as ready for review February 13, 2025 13:50
@davidhewitt
Copy link
Collaborator Author

@samuelcolvin I have tested this all the way up the stack e.g. in pydantic/pydantic#11439

I am satisfied that the performance here is fine and we should merge this. In core we can do fancy iteration if we want, but they don't add much benefit and do add a lot of complexity so I am not rushing to merge that. (Eventually we might want the core changes if we move to jiter iteration in core.)

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

LGTM

@davidhewitt davidhewitt merged commit 4023ba1 into main Feb 13, 2025
25 of 26 checks passed
@davidhewitt davidhewitt deleted the dh/simpler-value branch February 13, 2025 14:03
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