Skip to content
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

Don't allow simultaneous memory and CPU profiling #3922

Merged
merged 3 commits into from
Aug 12, 2020

Conversation

umanwizard
Copy link
Contributor

@umanwizard umanwizard commented Aug 12, 2020

See tikv/pprof-rs#36. Running the pprof CPU profiler at the same time as the jemalloc profiler is apparently UB. It reliably causes deadlocks in Materialize if the jemalloc sampling rate is aggressive enough (e.g. 2^14).

If another user accesses /prof while a CPU profile is running, this will have the unfortunate side effect of hanging their page load until the profile is done. We can improve that UX as a follow-up, but it requires a bit of refactoring (it's not as simple as just calling try_lock) and since it's a purely cosmetic issue, I didn't want it to block this UB safety issue.


This change is Reviewable

Comment on lines +77 to +79
// SAFETY: We ensure above that memory profiling is off.
// Since we hold the mutex, nobody else can be turning it back on in the intervening time.
let stacks = unsafe { prof_time(Duration::from_secs(10), 99, merge_threads) }.await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth adding an explicit drop(_ctl_lock); after this to ensure that we maintain ownership of the lock in the face of refactorings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@umanwizard umanwizard merged commit 69108af into MaterializeInc:main Aug 12, 2020
@umanwizard umanwizard deleted the main branch August 12, 2020 15:46
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.

3 participants