-
Notifications
You must be signed in to change notification settings - Fork 292
make error "duplicate" cheaper #950
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
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #950 +/- ##
==========================================
- Coverage 93.83% 93.80% -0.03%
==========================================
Files 105 105
Lines 15469 15468 -1
Branches 25 25
==========================================
- Hits 14515 14510 -5
- Misses 948 952 +4
Partials 6 6
Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #950 will improve performances by 26.67%Comparing Summary
Benchmarks breakdown
|
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes to tests and good performance improvement sounds great to me!
@@ -20,8 +22,8 @@ pub enum JsonInput { | |||
Array(JsonArray), | |||
Object(JsonObject), | |||
} | |||
pub type JsonArray = Vec<JsonInput>; | |||
pub type JsonObject = LazyIndexMap<String, JsonInput>; | |||
pub type JsonArray = Arc<SmallVec<[JsonInput; 8]>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we can have more than 8 items right? Is 8 just the default that gets allocated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, how smallvec
works is you pick a number of entries (in this case 8) which will be stored inline in the struct, and beyond that size it will instead use a heap allocation.
Change Summary
For #883 - replaces
ValLineError::duplicate
withValLineError::into_owned
which avoids the need to convertJsonInput
to Python data when handling errors.This is made possible by changing
JsonInput
to haveArc
on the array and object variants. I usedSmallVec
inside those variants to avoid double-allocating for small lists & maps. Overall this shouldn't impact performance much, ideally will make it a bit tidier in #883.Related issue number
N/A
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @samuelcolvin