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

move enabling fast TLS from jl_load_repl to jl_load_libjulia_internal #43655

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

KristofferC
Copy link
Member

From my understanding, enabling fast TLS right now only happens when the REPL is loaded. There are many cases where this might not be the case, for example if one creates "apps" using PackageCompiler where a custom executable is created. This moves the code to the jl_load_libjulia_internal which is annotated __attribute__((constructor)) and should therefore run as soon as the library is loaded.

@KristofferC KristofferC added backport 1.7 bugfix This change fixes an existing bug labels Jan 4, 2022
@oscardssmith
Copy link
Member

why is this a bugfix?

@vchuravy
Copy link
Member

vchuravy commented Jan 4, 2022

why is this a bugfix?

It is a performance regression on 1.7

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense on the level that I understand this code! Thanks Kristoffer and Jameson! :)

@staticfloat staticfloat merged commit f397b8b into master Jan 4, 2022
@staticfloat staticfloat deleted the kc/fast_tls branch January 4, 2022 19:18
KristofferC added a commit that referenced this pull request Jan 5, 2022
@KristofferC KristofferC mentioned this pull request Jan 5, 2022
23 tasks
Comment on lines +242 to +246
if (fptr == NULL || key == NULL) {
jl_loader_print_stderr("ERROR: Cannot find jl_get_pgcstack_static(), must define this symbol within calling executable!\n");
exit(1);
}
jl_pgcstack_setkey(fptr, key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert seems false?

Suggested change
if (fptr == NULL || key == NULL) {
jl_loader_print_stderr("ERROR: Cannot find jl_get_pgcstack_static(), must define this symbol within calling executable!\n");
exit(1);
}
jl_pgcstack_setkey(fptr, key);
if (fptr != NULL && key != NULL)
jl_pgcstack_setkey(fptr, key);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to #43689

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Valentin!

@vchuravy
Copy link
Member

vchuravy commented Jan 6, 2022

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants