Skip to content

Use PyInt for inequality and multiple of checks #634

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

Merged
merged 6 commits into from
May 26, 2023
Merged

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin commented May 25, 2023

This degrades performance in the case of inequality checks by around 30%, but if prevents issues with ints over i64::MAX.

Selected Reviewer: @dmontagu

@codspeed-hq
Copy link

codspeed-hq bot commented May 25, 2023

CodSpeed Performance Report

Merging #634 use-PyInt (89c310e) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 121 untouched benchmarks

🆕 2 new benchmarks
⁉️ 0 dropped benchmarks

Benchmarks breakdown

Benchmark main use-PyInt Change
🆕 test_int_range N/A 18.5 µs N/A
🆕 test_int_range_json N/A 19.7 µs N/A

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

Merging #634 (4d8f3c4) into main (6220455) will decrease coverage by 0.12%.
The diff coverage is 87.30%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #634      +/-   ##
==========================================
- Coverage   94.10%   93.98%   -0.12%     
==========================================
  Files          99       99              
  Lines       13561    13591      +30     
  Branches       25       25              
==========================================
+ Hits        12762    12774      +12     
- Misses        793      811      +18     
  Partials        6        6              
Impacted Files Coverage Δ
src/validators/union.rs 86.47% <ø> (ø)
src/input/input_json.rs 90.11% <57.14%> (-1.05%) ⬇️
src/input/return_enums.rs 88.42% <57.14%> (-0.53%) ⬇️
src/input/parse_json.rs 99.20% <80.00%> (-0.80%) ⬇️
src/input/shared.rs 96.42% <92.30%> (-3.58%) ⬇️
src/input/input_python.rs 98.36% <100.00%> (ø)
src/validators/int.rs 100.00% <100.00%> (ø)
src/validators/literal.rs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


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 6220455...4d8f3c4. Read the comment docs.

@samuelcolvin
Copy link
Member Author

please review.

Copy link
Collaborator

@dmontagu dmontagu left a comment

Choose a reason for hiding this comment

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

Would really prefer if bigints were just handled more transparently, but this looks fine to me if it's improving other issues.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good overall

@samuelcolvin
Copy link
Member Author

samuelcolvin commented May 26, 2023

Ask and thou shall receive - added BigInt support, supports parsing ints ups to 4300 characters - matching the constraints recently added in python.

I've also used BigInt for inequality checks and thereby improved the performance on that.

├────────────────────┼─────────────────────────────────────┼────────────────────┼───────────────────┼───────────┤
│  int_range         │  int_range                          │             122.0  │            107.2  │  -12.14%  │
│                    │  int_range_json                     │             158.4  │            116.1  │  -26.66%  │
└────────────────────┴─────────────────────────────────────┴────────────────────┴───────────────────┴───────────┘

@samuelcolvin samuelcolvin merged commit 60ed51f into main May 26, 2023
@samuelcolvin samuelcolvin deleted the use-PyInt branch May 26, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants