jiter icon indicating copy to clipboard operation
jiter copied to clipboard

feat: Make num-bigint optional

Open fasterthanlime opened this issue 1 year ago • 2 comments

Closes #124

Apologies for the few TOML reformats that slipped through, I can get rid of those if needed.

A few statics got turned into consts and moved inline with the BigInt code to avoid "unused variable" warnings.

I would recommend running cargo hack --feature-powerset check in CI, from https://lib.rs/crates/cargo-hack


A "default features build" (with num-bigint) is 4.7s at best for me:

CleanShot 2024-08-16 at 17 40 21@2x

Command used:

cargo clean ; cargo build --package jiter --quiet --timings && w3m -dump target/*timings/*timing.html | grep 'Total time.*$' --color=always -B3

And a "no default features" build is just under two seconds:

CleanShot 2024-08-16 at 17 41 36@2x

Command used:

cargo clean ; cargo build --package jiter --quiet --timings --no-default-features && w3m -dump target/*timings/*timing.html | grep 'Total time.*$' --color=always -B3

fasterthanlime avatar Aug 16 '24 15:08 fasterthanlime

CodSpeed Performance Report

Merging #130 will improve performances by 10.75%

Comparing bearcove:optional-num-bigint (c611089) with main (dd25fd0)

Summary

⚡ 1 improvements ✅ 72 untouched benchmarks

Benchmarks breakdown

Benchmark main bearcove:optional-num-bigint Change
python_parse_string_array_not_cached 40.1 µs 36.2 µs +10.75%

codspeed-hq[bot] avatar Aug 16 '24 15:08 codspeed-hq[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.91%. Comparing base (dd25fd0) to head (c611089). Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #130   +/-   ##
=======================================
  Coverage   94.91%   94.91%           
=======================================
  Files          12       12           
  Lines        1908     1909    +1     
=======================================
+ Hits         1811     1812    +1     
  Misses         97       97           
Files with missing lines Coverage Δ
crates/jiter/src/number_decoder.rs 93.26% <100.00%> (+0.02%) :arrow_up:
crates/jiter/src/value.rs 97.11% <ø> (ø)

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 dd25fd0...c611089. Read the comment docs.

codecov[bot] avatar Aug 16 '24 15:08 codecov[bot]

@fasterthanlime could you rebase please?

Done!

fasterthanlime avatar Sep 09 '24 14:09 fasterthanlime

(Is there a way to "Allow edits by maintainers" after the fact?)

fasterthanlime avatar Sep 09 '24 14:09 fasterthanlime

Thanks so much.

(Is there a way to "Allow edits by maintainers" after the fact?)

I think there might be a way, perhaps a checkbox once if you edit the PR body?

Anyway, happy to merge this once @davidhewitt has a quick check.

samuelcolvin avatar Sep 09 '24 20:09 samuelcolvin

Super, thanks very much!

Thanks for the review+merge! Can you tag me when this lands on crates.io?

(By the way I recommend the excellent release-plz for release automation, in case you're interested)

fasterthanlime avatar Sep 12 '24 11:09 fasterthanlime

I've raised a followup PR for this one as it looks like num-bigint is always pulling in pyo3 as well: https://github.com/pydantic/jiter/pull/160

cetra3 avatar Nov 06 '24 03:11 cetra3