-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add hook to initialize Julia on-the-fly during thread adoption #56334
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
Conversation
2f6a6c4 to
34fe416
Compare
|
Looks like this ironically brokes the trimming test. Any idea what's up @gbaraldi ? |
231560e to
3db38f9
Compare
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.
Thanks for splitting this for easy review!
I'm not a huge fan of the breaking change to jl_adopt_thread (a public, albeit dangerous-to-use function) - I think it's confusing that it now asks you for a parameter it almost never uses.
Other than that, changes look good to me 👍 thanks @gbaraldi
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.
I am not very happy about injecting all of this new code into every cfunction we have. I think you need to actually initialize pgcstack_func_slot with something
|
Yeah, I was thinking of some way of making this optional on sysimages. I guess we can add an attr to the sysimage module? The pgcstack_func_slot gets initialized by the runtime |
Is your concern code size, or performance overhead for the extra branch? |
|
Size, affecting speed and maintainability if we add other checks and atomics eventually, for what is always the same slow path code |
ce8dba4 to
6aeab74
Compare
|
I took the liberty of pushing a much simpler strategy of just initializing This no longer requires changes to PTLS codegen |
6aeab74 to
21fecef
Compare
This should be a dramatically simpler implementation, by just setting up the `jl_pgcstack_func_slot` to have a usable function pointer by default and then performing library introspection based on `__builtin_return_address` in the runtime. This avoids needing to modify codegen for `julia.get_pgcstack` at all.
21fecef to
452d6b9
Compare
This adds `jl_pgcstack_default_func` to allow an uninitialized runtime to make it into `jl_autoinit_and_adopt_thread` The init changes themselves are left to #57498 --------- Co-authored-by: Cody Tapscott <[email protected]>
The big changes are moving initialization to when someone calls into julia, making it all automatic. (It still supports manually initializing the runtime). I'm not done with this because i still need to figure out how to embed runtime arguments. Currently it's piped as a string but it's not clear yet if that's the best way forward.We maybe want to make the changes to adopt thread handling only exist behind a flag. Though that potentially doesn't add much because essentially this adds a very predictable null check to ccallableEdit: This PR has been partially split. It no longer makes any changes to PTLS codegen, and instead just adds
jl_pgcstack_default_functo allow an uninitialized runtime to make it intojl_autoinit_and_adopt_threadThe init changes themselves are left to #57498