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 fiber safety to crystal/once #15370

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jan 24, 2025

Removes the global Mutex (previously only used when the preview_mt flag is defined) that would prevent concurrent initializers to run in parallel, while still being able to detect reentrancy.

It works by keeping a list of pending operations (one per flag pointer) protected by a global lock that doesn't block other initializers from running.

Using a linked-list of stack allocated structs may sound inefficient compared to a Hash but there should usually not be many concurrent / parallel / nested lazy initializations that following a few pointers would be significant compared to calculating a hash and (re)allocating memory.

This isn't very important for the initialization of class vars and constants (happens sequentially at the start of the program, before we even start threads) but we plan to reuse the feature to implement #14905 where the lazy initialization can kick in at any time.

Follow up to #15369 and #15371

CREDITS: @BlobCodes wrote the initial implementation in master...BlobCodes:crystal:perf/crystal-once-v2. I mostly removed the fences (SpinLock already deals with memory order), made it compatible with both the enum or bool variants, where the enum allows to skip over checking the operations when processing —maybe not very significant?

I shall read his code much more carefully now 🙇

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 27, 2025

Possible change:

Since we assume the list of operations should usually be empty or only have a few entries at any time, we could drop the Crystal::Once::State enum to keep one unique Crystal::Once implementation (simpler). Only the __crystal_once fun would be different in the end (the legacy one still takes a state).

We might want to keep the flag as an Int8* or revert back to a Bool*. DONE. As outlined by #15387 having a single type is much easier.

Possible evolution:

We can use the simpler Fiber::Queue from #15345 instead of PointerLinkedList to keep the list of waiting fibers in Crystal::Once::Operation: no need for a doubly linked list with arbitrary deletion.

ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Jan 28, 2025
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Jan 28, 2025
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Jan 28, 2025
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 30, 2025

As demonstrated by #15387 this is broken on Windows.

I assume this is because we always start threads on Win32 yet Crystal::SpinLock is a NOOP without the preview_mt flag.

We can always enable the MT safety of Crystal::SpinLock for win32, or we can inline the atomic in Crystal::Once 🤔

ysbaddaden and others added 5 commits January 30, 2025 10:09
So we can use the getter and property macros in crystal/once
Removes the global mutex that prevents concurrent initializers to run,
while still being able to detect reentrancy, by keeping a list of
pending operations, protected by a global lock that doesn't block other
initializers from running.

Using a linked-list of stack allocated operations may sound inefficient
compared to a `Hash` but there should usually not be many concurrent or
parallel lazy initialization that following a few pointers will matter
much in practice.

This isn't very important for the eager initialization of class vars and
constants that happen sequentially at the start of the program and
before we even start threads, but we plan to reuse the feature to
protect lazy initialization of class variables (i.e. the `class_getter`
and `class_property` macros) when it would matter more.

Introduces the `Crystal::Once` namespace. The initialization entry is
now `Crystal::Once.init` instead of `Crystal.once_init`.

Co-authored-by: David Keller <[email protected]>
This avoids having two implementations and it will simplify the call
site when we protect class getter/property macros with lazy
initializers: the flag is always a Bool not either a
Crystal::Once::State or a Bool depending on the compiler.
@ysbaddaden ysbaddaden force-pushed the refactor/add-fiber-safety-to-crystal-once branch from 94745fd to cb536e9 Compare January 30, 2025 09:49
@ysbaddaden ysbaddaden marked this pull request as ready for review January 30, 2025 09:49
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

1 participant