Rework initialization of constants & class variables #15333
Conversation
|
This should have a |
Co-authored-by: David Keller <[email protected]> Based on the PR by @BlobCodes: crystal-lang#15216 The performance improvement is two-fold: 1. the usage of a i8 instead of an i1 boolean to have 3 states instead of 2, which permits to quickly detect recursive calls without an array; 2. inline tricks to optimize the fast and slow paths. Unlike the PR: 1. Doesn't use atomics: it already uses a mutex that guarantees acquire release memory ordering semantics, and __crystal_once_init is only ever called in the main thread before any other thread is started. 2. Removes the need for a state maintained by the compiler, yet keeps forward and backward compatibility (both signatures are supported).
Co-authored-by: David Keller <[email protected]> @BlobCodes: I noticed that adding this code prevents LLVM from re-running the once mechanism multiple times for the same variable. Modified to avoid an undefined behavior when the assumption doesn't hold that doubles as a safety net (print error + exit).
Co-authored-by: David Keller <[email protected]> @BlobCodes: I think it would be better to print the bug message in `Crystal.once` instead of `__crystal_once` to reduce complexity at the callsite. The previous unreachable method can then be used in the inlined `__crystal_once` so LLVM also knows it doesn't have to re-run the method. It's now even safe because `Crystal.once` would panic if it failed; it should already be impossible, but let's err on the safe side.
ysbaddaden
force-pushed
the
refactor/crystal-once
branch
from
d1de2eb to
ce6e9a2
Compare
|
Rebased to add the |
|
I think it's not registering in the interface. Maybe it has to be on the last line 🤔 |
|
FWIW we could also add the co-author attribution in the squash commit when we merge this PR (though we'd need to remember that, so it's best to already add it to the original commits 😅) |
|
I'm pretty sure the easiest way would be to cherry pick their commits into a branch, then push up your own. If there are commits from more than 1 author on a branch GH should automatically add the |
|
I added a commit to only enable the feature from crystal EDIT: why is CI getting completely broken because I changed the version comparison from |
|
Trying to work on #14905 I notice that this is still concurrency unsafe because we only enable |
Follow up and closes #15216 by @BlobCodes. Also incorporates the LLVM optimization from master...BlobCodes:crystal:perf/crystal-once-v2.
Similar to the original PR, this changes the
flagto have 3 states instead of 2. This led to change the signature of the__crystal_once[_init]functions that now take an Int8 (i8) instead of Bool (i1). In addition it drops the "once state" that isn't needed anymore.This requires a new compiler build to benefit from the improvement. The legacy versions of the
__crystal_once[_init]methods are still supported by both the stdlib and the compiler to keep both forward & backward compatibility (1.14- can build 1.15+ and 1.15+ can build 1.14-).Unlike the original PR, this one doesn't use any atomics: the mutex already protects the flag, and the mutex itself is explicitly initialized by the main thread before any other thread is started (i.e. no parallelism issues).
A follow-up could leverage
ReferenceStorageand.unsafe_constructto inline theMutexinstead of allocating in the GC heap. Along with #15330 then__crystal_once_initcould become allocation free, which could prove useful for such a core/low level feature.