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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

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 (a3c9ea3).
Report is 2 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...a3c9ea3. Read the comment docs.

Copy link

codspeed-hq bot commented Jan 23, 2025

CodSpeed Performance Report

Merging #184 will improve performances by 12.66%

Comparing dh/simpler-value (a3c9ea3) with main (9983083)

Summary

⚡ 2 improvements
✅ 68 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.6 µs N/A N/A
medium_response_jiter_value 41.3 µs 37.1 µs +11.38%
sentence_jiter_value 9 µs 8 µs +12.66%

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.

1 participant