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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

merged 6 commits into from
Feb 22, 2022

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Feb 10, 2022 •
edited
Loading

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

@NHDaly NHDaly requested a review from vtjnash February 10, 2022 20:16
@NHDaly NHDaly mentioned this pull request Feb 10, 2022
Closed
@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
@NHDaly
Fix thread-safety violation in Allocations Profiler:
e3fa8eb 
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
Approach two: Create a separate per-thread allocations backtrace buffer.
581374d
@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
vtjnash
vtjnash reviewed Feb 16, 2022
View reviewed changes
src/gc-alloc-profiler.cpp Outdated Show resolved Hide resolved
NHDaly and others added 2 commits February 16, 2022 22:52
@NHDaly @vtjnash
Update src/gc-alloc-profiler.cpp
e7fe3fb 
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@NHDaly @vtjnash
Update src/gc-alloc-profiler.cpp
77d401f 
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vilterp vilterp mentioned this pull request Feb 18, 2022
Merged
@vilterp
Copy link
Contributor

vilterp commented Feb 18, 2022 •
edited
Loading

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

@KristofferC KristofferC mentioned this pull request Feb 18, 2022
Merged
33 tasks
@IanButterworth
Copy link
Member

Is this ready to merge?

vtjnash
vtjnash reviewed Feb 22, 2022
View reviewed changes
src/gc-alloc-profiler.cpp Outdated Show resolved Hide resolved
vtjnash
vtjnash approved these changes Feb 22, 2022
View reviewed changes
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
@NHDaly
Fix thread-safety violation in Allocations Profiler: Create a new per…
a6f4b5e 
…-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
@NHDaly @vtjnash
@vilterp @staticfloat
Fix thread-safety violation in Allocations Profiler: Create a new per…
2292325 
…-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
@NHDaly @vtjnash
@vilterp @LilithHafner
Fix thread-safety violation in Allocations Profiler: Create a new per…
abd77eb 
…-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
@NHDaly @vtjnash @vilterp
Fix thread-safety violation in Allocations Profiler: Create a new per…
22bb755 
…-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 NHDaly mentioned this pull request Mar 21, 2022
Closed
NHDaly added a commit that referenced this pull request Apr 5, 2022
@NHDaly @vtjnash @vilterp
Fix thread-safety violation in Allocations Profiler: Create a new per…
a690e38 
…-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
Reviewers

@vilterp vilterp vilterp left review comments

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

allocation profiler not threadsafe
8 participants
@NHDaly @vilterp @IanButterworth @KristofferC @vtjnash @vchuravy @DilumAluthge @Sacha0