Skip to content

fix: allow lazy TLS init for threads started before profiling#531

Draft
nsavoire wants to merge 1 commit intomainfrom
nsavoire/allow_lazy_tls_init
Draft

fix: allow lazy TLS init for threads started before profiling#531
nsavoire wants to merge 1 commit intomainfrom
nsavoire/allow_lazy_tls_init

Conversation

@nsavoire
Copy link
Copy Markdown
Collaborator

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.

@nsavoire nsavoire force-pushed the nsavoire/allow_lazy_tls_init branch 2 times, most recently from 5ae34ef to 780ea42 Compare March 31, 2026 09:20
@pr-commenter
Copy link
Copy Markdown

pr-commenter bot commented Mar 31, 2026

Benchmark results for collatz

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.24.0+978f4be5.105195233 ddprof 0.24.0+780ea42a.105213418

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-collatz --preset cpu_only collatz_runner.sh same

@pr-commenter
Copy link
Copy Markdown

pr-commenter bot commented Mar 31, 2026

Benchmark results for BadBoggleSolver_run

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.24.0+978f4be5.105195233 ddprof 0.24.0+780ea42a.105213418

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-bad-boggle-solver BadBoggleSolver_run work 1000 same

return &state;
}
if (!state.initialized && !state.reentry_guard) {
// We are not inside pthread_getattr_np, so we can initialize the state
Copy link
Copy Markdown
Collaborator

@r1viollet r1viollet Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the state.reentry_guard captures also when we are in an allocation hook ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although this should not happen as we should never enter without a state, though this is a shared reentry mechanism

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reused state.reentry_guard since we don't use it when state is not yet initialized and the purpose is similar

@r1viollet
Copy link
Copy Markdown
Collaborator

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).
His proposal was to only perform the init on APIs where it is impossible for us to hold a lock (like inside a malloc hook).
This is a great idea!

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.
@nsavoire nsavoire force-pushed the nsavoire/allow_lazy_tls_init branch from 780ea42 to a9d5823 Compare March 31, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants