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

Add hook to initialize Julia on-the-fly during thread adoption #56334

Merged
merged 2 commits into from
Feb 26, 2025

Conversation

gbaraldi
Copy link
Member

@gbaraldi gbaraldi commented Oct 25, 2024

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 ccallable

Edit: This PR has been partially split. It no longer makes any changes to PTLS codegen, and instead just 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

@gbaraldi gbaraldi force-pushed the gb/refactor-juliac-init branch from 2f6a6c4 to 34fe416 Compare October 29, 2024 19:32
@gbaraldi gbaraldi marked this pull request as ready for review November 15, 2024 13:41
@topolarity
Copy link
Member

Looks like this ironically brokes the trimming test.

Any idea what's up @gbaraldi ?

Copy link
Member

@topolarity topolarity left a 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

@topolarity topolarity changed the title Refactor julia initialization to behave better when used as a dylib Add hook to initialize Julia on-the-fly during thread adoption Feb 21, 2025
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.

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

@gbaraldi
Copy link
Member Author

gbaraldi commented Feb 21, 2025

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

@topolarity
Copy link
Member

I am not very happy about injecting all of this new code into every cfunction we have.

Is your concern code size, or performance overhead for the extra branch?

@vtjnash
Copy link
Member

vtjnash commented Feb 22, 2025

Size, affecting speed and maintainability if we add other checks and atomics eventually, for what is always the same slow path code

@topolarity topolarity force-pushed the gb/refactor-juliac-init branch 2 times, most recently from ce8dba4 to 6aeab74 Compare February 22, 2025 21:49
@topolarity
Copy link
Member

I took the liberty of pushing a much simpler strategy of just initializing jl_pgcstack_func_slot and using __builtin_return_address for figuring out the library / entrypoint that triggered auto-init.

This no longer requires changes to PTLS codegen

@topolarity topolarity force-pushed the gb/refactor-juliac-init branch from 6aeab74 to 21fecef Compare February 22, 2025 21:55
@topolarity topolarity requested a review from vtjnash February 24, 2025 14:47
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.
@topolarity topolarity force-pushed the gb/refactor-juliac-init branch from 21fecef to 452d6b9 Compare February 26, 2025 11:55
@topolarity topolarity added the merge me PR is reviewed. Merge when all tests are passing label Feb 26, 2025
@topolarity topolarity merged commit 94486f1 into master Feb 26, 2025
8 checks passed
@topolarity topolarity deleted the gb/refactor-juliac-init branch February 26, 2025 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me PR is reviewed. Merge when all tests are passing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants