Profile: allocate buffer for n instruction pointers per thread #41821
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 tasks
timholy
approved these changes
Aug 7, 2021
vchuravy
approved these changes
Aug 8, 2021
vchuravy
reviewed
Aug 9, 2021
| @@ -40,7 +40,7 @@ end | |||
| init(; n::Integer, delay::Real)) | |||
|
|
|||
| Configure the `delay` between backtraces (measured in seconds), and the number `n` of | |||
| instruction pointers that may be stored. Each instruction pointer corresponds to a single | |||
| instruction pointers that may be stored per thread. Each instruction pointer corresponds to a single | |||
There was a problem hiding this comment.
Maybe add a !!! compat note?
vchuravy
added
status:merge me
NHDaly
approved these changes
Aug 9, 2021
| @@ -40,7 +40,7 @@ end | |||
| init(; n::Integer, delay::Real)) | |||
|
|
|||
| Configure the `delay` between backtraces (measured in seconds), and the number `n` of | |||
| instruction pointers that may be stored. Each instruction pointer corresponds to a single | |||
| instruction pointers that may be stored per thread. Each instruction pointer corresponds to a single | |||
IanButterworth
force-pushed
the
ib/profile_samples_per_thread
branch
from
8080207
to
afca441
Compare
DilumAluthge
removed
the
status:merge me
|
It looks like this broke the linux32 build, as seen in the pre-merge CI |
|
Sorry about that. The only way I can think that this PR is causing the error below is if somehow the |
|
It is attempting to initialize profile space for 128 threads, and now failing (OOM) |
|
128 real threads, or is that a bug also? If that's real, then options seem:
I guess it's not reliable that a n-thread machine would have n-times more ram than a 1-thread machine, which this PR kind of assumes |
IanButterworth
commented
Aug 17, 2021
| """ | ||
| function init(; n::Union{Nothing,Integer} = nothing, delay::Union{Nothing,Real} = nothing) | ||
| n_cur = ccall(:jl_profile_maxlen_data, Csize_t, ()) | ||
| delay_cur = ccall(:jl_profile_delay_nsec, UInt64, ())/10^9 | ||
| if n === nothing && delay === nothing | ||
| return Int(n_cur), delay_cur | ||
| end | ||
| nnew = (n === nothing) ? n_cur : n | ||
| nthreads = Sys.iswindows() ? 1 : Threads.nthreads() # windows only profiles the main thread | ||
| nnew = (n === nothing) ? n_cur : n * nthreads |
There was a problem hiding this comment.
There's also a bug here. This method passes to the method below, so this was multiplying by nthreads twice. I'm about to open a PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given that profiling samples all threads (except windows that only profiles the main thread) it seems to make sense to allocate nsamples per thread (except windows).