Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.2k
Schema cleaning: skip unnecessary copies during schema walking#10286
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f519d94
to80e1de2
Comparecloudflare-workers-and-pagesbot commentedSep 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Deploying pydantic-docs with |
Latest commit: | 7967d58 |
Status: | ✅ Deploy successful! |
Preview URL: | https://aa61922c.pydantic-docs.pages.dev |
Branch Preview URL: | https://schema-cleaning-no-copy.pydantic-docs.pages.dev |
codspeed-hqbot commentedSep 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#10286 willnot alter performanceComparing Summary
|
80e1de2
to9d4745c
Comparegithub-actionsbot commentedSep 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Coverage reportClick to see where and how coverage changed
This report was generated bypython-coverage-comment-action |
@Viicos is this ready for review? |
Could you add a note to this PR re incompatibility with a cache based approach? I'm keen to accept this as is for now, then revert if we come up with a good caching solution pre our next release. |
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.
This looks good to me.
As mentioned on the schema walking performance analysis issue, I think there are more improvements to be made here, but this is a good start.
Thanks@Viicos!
22ba7fa
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Related issue:
A new
copy
argument is added to_WalkCoreSchema
, defaulting toTrue
. In the relevant steps of schema cleaning, no copy is performed.Flamegraph:
py-spy record -o k8s_v2_no_copy.json -f speedscope -- python k8s_v2.py
Time order:
Left heavy:
The left heavy schema clearly shows that
collect_refs
(the first handler function to be used withwalk_core_schema
) takes most of the time, and the other ones now represents almost nothing.Seems like ~1 second can be saved. Not bad, but not that great either. Perhaps dict copies aren't that expensive? Unsurprisingly, memory consumption is the same (~600MiB in the end):
memray run -o k8s_v2_no_copy.bin k8s_v2.py && memray flamegraph k8s_v2_no_copy.bin