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

Replace definitions Vec with OnceLock slots #992

Merged
merged 4 commits into from
Sep 28, 2023
Merged

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Sep 26, 2023

Change Summary

This PR reworks the way definitions work so that instead of storing them in a global Vec they are shared inside Arcs with a OnceLock internally meaning that there is one opportunity to build them.

The hope is that this transformation may pave the way for reusing SchemaValidator inside core schema as a compiled subgraph to reduce memory consumption and simplify the defs transformations needed in pydantic.

Related issue number

Related to pydantic/pydantic#7617

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #992 (5f14973) into main (1a966d5) will increase coverage by 0.05%.
The diff coverage is 85.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #992      +/-   ##
==========================================
+ Coverage   92.98%   93.04%   +0.05%     
==========================================
  Files         106      106              
  Lines       15827    15701     -126     
  Branches       35       35              
==========================================
- Hits        14717    14609     -108     
+ Misses       1103     1085      -18     
  Partials        7        7              
Files Coverage Δ
src/py_gc.rs 100.00% <100.00%> (ø)
src/serializers/extra.rs 99.08% <ø> (-0.03%) ⬇️
src/serializers/mod.rs 98.47% <100.00%> (-0.02%) ⬇️
src/serializers/shared.rs 79.88% <100.00%> (ø)
src/serializers/type_serializers/dataclass.rs 92.72% <100.00%> (ø)
src/serializers/type_serializers/definitions.rs 93.93% <100.00%> (+0.18%) ⬆️
src/serializers/type_serializers/model.rs 94.55% <100.00%> (ø)
src/validators/any.rs 100.00% <100.00%> (ø)
src/validators/arguments.rs 99.60% <100.00%> (-0.01%) ⬇️
src/validators/bool.rs 100.00% <100.00%> (ø)
... and 43 more

... 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 1a966d5...5f14973. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 26, 2023

CodSpeed Performance Report

Merging #992 will improve performances by 14.1%

Comparing dh/faster-definitions (5f14973) with main (1a966d5)

Summary

⚡ 1 improvements
✅ 137 untouched benchmarks

Benchmarks breakdown

Benchmark main dh/faster-definitions Change
test_generator_rust 35.5 µs 31.1 µs +14.1%

@davidhewitt davidhewitt force-pushed the dh/faster-definitions branch 2 times, most recently from ea8ed50 to 705618e Compare September 26, 2023 15:46
@davidhewitt
Copy link
Contributor Author

I've pushed a follow-up commit which removes validator cloning, to close #843

@davidhewitt davidhewitt linked an issue Sep 26, 2023 that may be closed by this pull request
@davidhewitt
Copy link
Contributor Author

please review

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.

Given the lack of changes to tests this looks good to me!

@davidhewitt davidhewitt merged commit a8fb1e3 into main Sep 28, 2023
30 checks passed
@davidhewitt davidhewitt deleted the dh/faster-definitions branch September 28, 2023 08:45
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.

Allocations overhead ?
3 participants