PoC: Replace Thread::Local(T) with Thread::LocalStorage (alternative)#16168
Closed
ysbaddaden wants to merge 6 commits intocrystal-lang:masterfrom
Closed
PoC: Replace Thread::Local(T) with Thread::LocalStorage (alternative)#16168ysbaddaden wants to merge 6 commits intocrystal-lang:masterfrom
Thread::Local(T) with Thread::LocalStorage (alternative)#16168ysbaddaden wants to merge 6 commits intocrystal-lang:masterfrom
Conversation
Relies on a macro to inject typed instance variables into the container, and optional destructors as class variables. The overall design is very simple and accessing a thread local is basically dereferencing the actual thread local and accessing an ivar. One drawback is that the compiler doesn't drop unused ivars, so as soon as an executable needs a thread local, it will allocate the whole type, even though the program only uses a few of them. Since we don't intent to have lots of thread locals, this ain't much of an issue. Another drawback is that destructors will always be registered, whatever if we actually need the thread local, which may lead to always link libpcre2 for example, because the destructors will link the external free symbols, which can be problematic. Maybe weak symbols could help?
Fixes the main drawback of this approach by setting the destructor callback when we set the thread local, so the destructor will only be set if the program uses the thread local.
ysbaddaden
deleted the
feature/custom-thread-local-storage-with-container-type
branch
Collaborator
Author
|
Superseded by #16173. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This is an alternative to #16029 that uses a static "container type" instead of a table of void pointers. It roughly moves hard coded values from
Thread.currentinto a dedicated type (allocated when needed) and relies on a macro to generate some code.It has some advantages, most notably a much simpler implementation and the ability to store any type (not just pointers and nilable references).
It also has some disadvantages, for example it depends on the macro and we lose flexility (must use the generated class method, can only create globals, no runtime dynamism), any declared thread local will grow the container type regardless of actual usage, which in turn requires to set the destructor callback to when we set the TLS... though now that I think about it, I should just use a wrapper class with a finalizer instead 🤦
Credits where its due: the initial idea is from @BlobCodes. See #15616 (comment) and #15889.