fix: allow lazy TLS init for threads started before profiling#531
Conversation
5ae34ef to
780ea42
Compare
Benchmark results for collatzParameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. See unchanged results
|
Benchmark results for BadBoggleSolver_runParameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. See unchanged results
|
|
Nicolas had a smart idea, the scenario we are worried about is doing an init while we already have a lock (allocation) held. This can happens when you are doing an mmap within a malloc (especially as we can not hook all mallocs). |
When profiling starts on a process with already-running threads calling pthread_getattr_np, the allocation tracker's TLS state may not be initialized yet. This adds a pthread_getattr_np hook that guards against deadlock withthe following sequence: (pthread_getattr_np -> malloc -> tracker -> init_tl_state -> save_context -> pthread_getattr_np deadlock) and enables lazy TLS init in get_tl_state() when safe.
780ea42 to
a9d5823
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9d5823d65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // malloc implementation and cause a deadlock when pthread_getattr_np (which | ||
| // can call malloc) is called by tls state initialization. | ||
| AllocTrackerHelperImpl() | ||
| : _tl_state{ddprof::AllocationTracker::get_tl_state(mmap_alloc)}, |
There was a problem hiding this comment.
is the point not to have get_tl_state(!mmap_alloc)
I guess this is what codex is saying ?
There was a problem hiding this comment.
oups,indeed I inverted the condition
There was a problem hiding this comment.
the fact that tests are still OK with this inverted is strange ? Or just lucky ?
There was a problem hiding this comment.
that's because there is no test for "mmap hook called inside malloc without malloc hook being called"
I added one
9e6c139 to
cfb933a
Compare
There was a problem hiding this comment.
nit, this requires false ? to avoid running init in child
There was a problem hiding this comment.
I changed all get_tl_state() to get_tl_state(false) to ensure that the behaviour remains the same in the test.
r1viollet
left a comment
r1viollet
left a comment
There was a problem hiding this comment.
Thanks for adjusting tests.
LGTM
2 participants
What does this PR do?
When profiling starts on a process with already-running threads calling pthread_getattr_np, the allocation tracker's TLS state may not be initialized yet. This adds a pthread_getattr_np hook that guards against deadlock withthe following sequence:
(pthread_getattr_np -> malloc -> tracker -> init_tl_state -> save_context -> pthread_getattr_np deadlock) and enables lazy TLS init in get_tl_state() when safe.
Motivation
Allow allocation profiling of existing threads when profiler is not started at process startup.