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

Fix thread-safety violation in Allocations Profiler: Create a new per-thread backtrace buffer in ptls #44116

Merged
merged 6 commits into from
Feb 22, 2022

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Feb 10, 2022

Another approach to fix #44099.

This is an alternative approach to #44114, but it creates a separate per-thread allocations backtrace buffer, to avoid potential sharing issues.

CC: @vilterp

@vilterp
Copy link
Contributor

vilterp commented Feb 10, 2022

Nice thanks. I’m a bit more comfortable with this, but need more input 🤷‍♂️

@NHDaly
Copy link
Member Author

NHDaly commented Feb 10, 2022

Yeah, same here. Would be happy to get input from someone else, like @vtjnash or @vchuravy. Otherwise, i'd prefer this approach too, i think

@NHDaly NHDaly force-pushed the nhd-alloc-profiler-thread-safety--new-buffer branch from 406d23c to d0677a0 Compare February 15, 2022 01:22
Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.
@NHDaly NHDaly force-pushed the nhd-alloc-profiler-thread-safety--new-buffer branch from d0677a0 to 581374d Compare February 15, 2022 01:23
@vchuravy vchuravy added this to the 1.8 milestone Feb 15, 2022
@KristofferC KristofferC added the backport 1.8 Change should be backported to release-1.8 label Feb 16, 2022
src/gc-alloc-profiler.cpp Outdated Show resolved Hide resolved
NHDaly and others added 2 commits February 16, 2022 22:52
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vilterp
Copy link
Contributor

vilterp commented Feb 18, 2022

Build failing with a type error; fixed in #44235 which merges into this

@IanButterworth
Copy link
Member

Is this ready to merge?

src/gc-alloc-profiler.cpp Outdated Show resolved Hide resolved
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM

@vilterp
Copy link
Contributor

vilterp commented Feb 22, 2022

Thanks Jameson :) I don't have permissions to merge this and @NHDaly is out on vacation, so if someone with permissions could merge that would be great.

I think we'll also want to backport to 1.8 as well. Thanks!

@vchuravy vchuravy added the merge me PR is reviewed. Merge when all tests are passing label Feb 22, 2022
@KristofferC
Copy link
Member

I think we'll also want to backport to 1.8 as well.

It already has the label so it will get backported.

@vilterp
Copy link
Contributor

vilterp commented Feb 22, 2022

@KristofferC oh I see, great; you're way ahead of me :)

@Sacha0 Sacha0 merged commit 4e57966 into master Feb 22, 2022
@Sacha0 Sacha0 deleted the nhd-alloc-profiler-thread-safety--new-buffer branch February 22, 2022 20:33
@vilterp
Copy link
Contributor

vilterp commented Feb 22, 2022

Thanks Sacha 🙏

KristofferC pushed a commit that referenced this pull request Feb 23, 2022
…-thread backtrace buffer in ptls (#44116)

* Fix thread-safety violation in Allocations Profiler:

Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.

* Approach two: Create a separate per-thread allocations backtrace buffer.

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* fix type error (#44235)

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Pete Vilter <7341+vilterp@users.noreply.github.com>
(cherry picked from commit 4e57966)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Feb 24, 2022
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Feb 25, 2022
staticfloat pushed a commit to JuliaCI/julia-buildkite-testing that referenced this pull request Mar 2, 2022
…-thread backtrace buffer in ptls (JuliaLang#44116)

* Fix thread-safety violation in Allocations Profiler:

Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.

* Approach two: Create a separate per-thread allocations backtrace buffer.

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* fix type error (JuliaLang#44235)

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Pete Vilter <7341+vilterp@users.noreply.github.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…-thread backtrace buffer in ptls (JuliaLang#44116)

* Fix thread-safety violation in Allocations Profiler:

Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.

* Approach two: Create a separate per-thread allocations backtrace buffer.

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* fix type error (JuliaLang#44235)

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Pete Vilter <7341+vilterp@users.noreply.github.com>
NHDaly added a commit that referenced this pull request Mar 21, 2022
…-thread backtrace buffer in ptls (#44116)

* Fix thread-safety violation in Allocations Profiler:

Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.

* Approach two: Create a separate per-thread allocations backtrace buffer.

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* fix type error (#44235)

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Pete Vilter <7341+vilterp@users.noreply.github.com>
NHDaly added a commit that referenced this pull request Apr 5, 2022
…-thread backtrace buffer in ptls (#44116)

* Fix thread-safety violation in Allocations Profiler:

Re-use the shared `ptls->bt_data` buffer from the thread-local storage
for the buffer, to ensure that each thread has a separate buffer.

This buffer is shared with the exception throwing mechanism, but is safe
to share since julia exception throwing never interleaves with
allocations profiling.

* Approach two: Create a separate per-thread allocations backtrace buffer.

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

* fix type error (#44235)

* Update src/gc-alloc-profiler.cpp

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Pete Vilter <7341+vilterp@users.noreply.github.com>
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.

allocation profiler not threadsafe
8 participants