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.
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
Allocation profiler #42768
Allocation profiler #42768
Changes from 105 commits
b49dd24
2fe1678
c370f5a
b8cffd3
c733cd6
964a8a4
aed5f5d
665cacd
7ed832e
371bb96
c8452ce
2d9ec5b
5cd096a
04e2b17
77ce036
33dde11
07d1009
6ec2fd0
f5f1be2
9fe1ad2
f768234
b11a0fe
c1faa44
671b88e
6932831
e164cc8
c736dbc
9407a91
f297d94
50dcb63
976d2bc
e1b2221
03a0102
7975c77
2c84bb9
b49606d
84745bf
a03cfe3
79427ca
9f63529
7d12971
a26f375
b5ab611
d53afa2
9de01c6
cdabd0e
ba3c0c8
ddf945c
e2a8ba3
ad1b0e0
a712267
ec6cc6f
83ef9ea
44073ef
b3e5102
0fd988b
0119ce3
9d06356
f0d7da4
cecf3a1
545d06c
e1f8684
7de1d5f
7c862da
2c271ab
2cb0a59
535c2fc
af8931c
f47253c
423830d
4a86866
d02a5b6
bfd6210
ff00317
e07e67d
ed4cb44
c27971b
48261b5
43276f4
8062b72
aeec3b6
00bb947
d3d868f
d352a80
3aaa9d9
db64f63
5c25bcd
4fec8dd
9a4b7fb
5dc2beb
830e2ad
cd14357
0f39d48
e98589b
107ece0
c4bebea
f4f1a62
452c7cd
3f219c7
b25005a
bf08801
8ec194b
41f169c
3fc80ac
9465507
76ad6f0
dd693cc
4deaf6c
13a85de
68e1a15
b5f636a
b16b610
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not thread safe. You should be (re)using the existing buffer in the ptls to be safe here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Yeah, we need a buffer per thread…
Did you mean to (re)use this buffer? https://github.com/vilterp/julia/blob/c12aca890a8ae387d11db0b54351f8b61305c00b/src/julia_threads.h#L247
It seems like we should add our own (here or as a global) to avoid colliding with that… We've shied away from adding stuff to ptls to avoid breaking stuff or adding overhead, but maybe it's better than globals?
Funny our multithreaded test didn't catch this; maybe the stack traces were just garbage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as you avoid re-using this buffer in the interval between an error filling this buffer, and then allocating the exception stack, I think you should be okay here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me a little nervous to reuse the buffer for two things, but it is space-efficient! I guess it should be fine like you say…
filed #44099 to track this; we'll write up a PR soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backtrace would be essentially the same, just a bit longer perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @vtjnash for pointing this out! 😅 haha it feels pretty silly that we missed that. Thanks for the help!!
The suggestion to use a buffer in ptls makes sense. I agree with @vilterp that sharing that buffer makes me slightly nervous, but reading through your description that sounds totally fine 👍 cool, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the CPU profiler use that buffer at all? If someone is running CPU profiling and Allocs profiling at the same time, is there a chance it'll interrupt our thread while we were in the middle of using the buffer and we'll get any problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Profiling using a different buffer entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question: is it okay to write over this buffer, since it seems like it's used to scan for roots?:
julia/src/task.c
Lines 344 to 345 in 7ccc83e
julia/src/task.c
Lines 618 to 623 in 7ccc83e
I think that it's fine, from what i can read, but just want to double check one more time.
Also, i've opened a PR for this, here: #44114