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

feat: Make num-bigint optional #130

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

fasterthanlime
Copy link
Contributor

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

Copy link

codspeed-hq bot commented Aug 16, 2024

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%

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️
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.

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, thanks so much.

@davidhewitt you happy with this?

I tried to merge with main to fix the fuzz failures, but you've blocked contributions from maintainers, @fasterthanlime could you rebase please?

@fasterthanlime
Copy link
Contributor Author

@fasterthanlime could you rebase please?

Done!

@fasterthanlime
Copy link
Contributor Author

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

@samuelcolvin
Copy link
Member

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.

Copy link
Collaborator

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Super, thanks very much!

@davidhewitt davidhewitt merged commit b09b969 into pydantic:main Sep 12, 2024
24 checks passed
@fasterthanlime
Copy link
Contributor Author

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)

@cetra3
Copy link
Contributor

cetra3 commented Nov 6, 2024

I've raised a followup PR for this one as it looks like num-bigint is always pulling in pyo3 as well: #160

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.

Would you consider making bigint support optional?
4 participants