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

Initialize Fiber with an explicit stack #15409

Merged

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Feb 4, 2025

Doesn't change the public API (the stack is still taken from the current scheduler's stack pool), but introduces an undocumented initializer that takes the stack and stack_bottom pointers.

This will allow a few scenarios, mostly for RFC 2:

  • start fibers with a fake stack when we don't need to run the fibers, for example during specs that only need fiber objects (and would leak memory since we only release stacks after the fiber has run);
  • during a cross execution context spawn, we can pick a stack from the destination context instead of the current context (so we can recycle stacks from fibers that terminated in the destination context).

Extracted from #15404
Follow up to #15434
Required by #15345

@ysbaddaden ysbaddaden self-assigned this Feb 4, 2025
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Feb 4, 2025
Doesn't change the public API (the stack is still taken from the current
scheduler's stack pool), but introduces an undocumented initializer that
takes the stack and stack_bottom pointers.

This will allow a few scenarios, mostly for RFC 2:

- start fibers with a fake stack when we don't need to run the fibers,
  for example during specs that only need fiber objects (and would leak
  memory since we only release stacks after the fiber has run);
- during a cross execution context spawn, we can pick a stack from the
  destination context instead of the current context (so we can recycle
  stacks from fibers that terminated in the desination context).
@straight-shoota
Copy link
Member

issue: I'm wondering if we need protection to prevent injected stacks to end up in the pool.
If you initialize a fiber with an explicit stack and that fiber does actually run, it would eventually release the stackpointer to the pool. This could unintentionally leak stub stacks into regular fibers and cause all kinds of weirdness.
Neither the fiber keeps track of where the stack comes from, nor do the fiber pools keep track to identify their stack pointers.

A possible solution for this problem would be to add a callback for releasing the stack.
An evolution could be the introduction of a Stack type which combines stack, stack_bottom and the cleanup callback.

@ysbaddaden
Copy link
Contributor Author

@straight-shoota I tried to extract a Fiber::Stack struct that holds the stack limits, and a flag to tell whether the stack is reusable (i.e. shall be released back into stack pool).

src/fiber/stack.cr Outdated Show resolved Hide resolved
Doesn't change the public API (the stack is still taken from the current
scheduler's stack pool), but introduces an undocumented initializer that
takes the stack and stack_bottom pointers.

This will allow a few scenarios, mostly for RFC 2:

- start fibers with a fake stack when we don't need to run the fibers,
  for example during specs that only need fiber objects (and would leak
  memory since we only release stacks after the fiber has run);
- during a cross execution context spawn, we can pick a stack from the
  destination context instead of the current context (so we can recycle
  stacks from fibers that terminated in the desination context).
Holds the stack limits and whether the stack can be reused (i.e.
released back into Fiber::StackPool).

Also abstracts accessing the first addressable pointer with 16-bytes
alignment (as required by most architectures) to pass to `makecontext`.
@ysbaddaden ysbaddaden force-pushed the feature/specify-fiber-stack branch from 9707185 to aae3498 Compare February 10, 2025 15:06
src/fiber/stack_pool.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Feb 10, 2025

It should be good now 🤞

@straight-shoota
Copy link
Member

question: What's the reason for reverting bytesize vs. bottom? I suppose it doesn't matter much either way, just curious.

@ysbaddaden
Copy link
Contributor Author

CI started failing with math overflow when creating the main fiber from the thread 🤷

@straight-shoota straight-shoota added this to the 1.16.0 milestone Feb 10, 2025
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Feb 10, 2025
@ysbaddaden ysbaddaden merged commit cb7782d into crystal-lang:master Feb 11, 2025
71 checks passed
@ysbaddaden ysbaddaden deleted the feature/specify-fiber-stack branch February 11, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants