-
Notifications
You must be signed in to change notification settings - Fork 412
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
fix(core): add level of indirection for provider.py contextvars #10525
base: main
Are you sure you want to change the base?
Conversation
|
interesting and promising |
BenchmarksBenchmark execution time: 2024-10-08 14:24:22 Comparing candidate commit 0712bbc in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 339 metrics, 53 unstable metrics. |
Could you run |
Datadog ReportBranch report: ✅ 0 Failed, 1142 Passed, 144 Skipped, 28m 48.41s Total duration (9m 30.07s time saved) |
…g/dd-trace-py into sanchda/make_contextvars_indirect
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.
Going to let @emmettbutler handle this review
releasenotes/notes/fix-contextvar-cloning-49adaf7fdf36e8fb.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: Emmett Butler <[email protected]>
@wconti27 thanks for bringing @emmettbutler into the discussion--can you toggle your review? Merges are currently blocked since your last review required changes. 🙇 |
/merge |
🚂 MergeQueue: waiting for PR to be ready This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
Whenever I see failing tests in CI for this PR, I've been pushing retry button. I've pressed the button enough number of times to believe those are actual failures. |
This merge request was unqueued If you need support, contact us on Slack #devflow! |
but what if it passes if I retry just one more time (jk, checking) |
Whenever a contextvar is reassociated, it causes the underlying HAMT data structure to clone a node. This clone operation requires de-referencing stored Python objects, which can cause segmentation faults if other libraries mis-manage the reference counts for their objects, causing them to be GC'd.
This patch stores a single wrapper object into the contextvar, then manipulates a reference within that wrapper in order to propagate our desired information. In our normal testing fixture, we cause as many as 69 realloc (and clone) events in a single process (I deduce this by patching cpython itself to produce a log). With this patch, that number is down to 1, and it doesn't originate from this provider.py
I have a standalone reproduction for the noted behavior here. The repro isn't very clever about how it manages the lifetimes of GC'd objects--the issues we see in the wild are a little bit more subtle, since they don't segfault during normal scope cleanup (unlike mine).
Checklist
Reviewer Checklist