Skip to content

Conversation

@straight-shoota
Copy link
Member

This is a much simpler alternative to #16329 which requires no refactoring. We already have a dedicated test job for multi-threading. It just doesn't use execution contexts yet. Adding that already gives great test coverage for the new feature.
It only runs std_spec but that should be fine. I wouldn't expect much benefit from running compiler specs as well.

There's not much point in keeping tests for only -Dpreview_mt without -Dexecution_context because we're not expecting to continue supporting the multi-threading preview with the old scheduler model.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Nov 7, 2025

There's a huge difference: with preview_mt alone we start multiple threads/schedulers automatically, so fibers will run in parallel, while execution_context only starts a single thread/scheduler and fibers only run concurrently.

There's value in testing the concurrent case, but we also want to test parallelism, so there should be two jobs:

  1. EC enabled and 1 worker (concurrent)
  2. EC enabled and N workers (parallel)

The second case needs either support in spec (stdlib) or at least in a spec helper (not using .default_workers_count because it defaults to the number of cpus):

{% if Fiber.has_constant?(:ExecutionContext) %}
  count = ENV["CRYSTAL_WORKERS"]?.try(&.to_i?) || 1
  Fiber::ExecutionContext.current.resize(count)
{% end %}

@straight-shoota
Copy link
Member Author

I think we need to add the EC resize to stdlib spec package so it can work in any spec not just one including some specific helper. I think that makes sense, because this behaviour is useful outside of stdlib specs. It practically only has an effect if CRYSTAL_WORKERS is set.
We might want to extract that as a separate change, though.

Apparently, Fiber::ExecutionContext can be defined, even without -Dexecution_context. But then it's missing .current: for example https://github.com/crystal-lang/crystal/actions/runs/19719679745/job/56499752283?pr=16339 😞
Looks like ExecutionContext is spilling somewhere when it's not used: GlobalQueue, Runnables, FiberCounter are defined, but no methods.


{% if Fiber.has_constant?(:ExecutionContext) && Fiber::ExecutionContext.class.has_method?(:current) %}
count = ENV["CRYSTAL_WORKERS"]?.try(&.to_i?) || 1
# FIXME: ExecutionContext.current should return ExecutionContext::Parallel
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been fixed in #16367.

@ysbaddaden
Copy link
Contributor

The namespace is leaking in std specs because of spec/std/fiber/execution_context/* spec files that always require GlobalQueue and Runnables.

@ysbaddaden
Copy link
Contributor

There should be a distinct PR to resize the default execution context in spec, and maybe we should think of a CLI arg to set the number of workers, that could default to ENV["CRYSTAL_WORKERS"]? || 1.

@straight-shoota
Copy link
Member Author

straight-shoota commented Nov 27, 2025

It would be really nice if we had a simple test to check whether we're using execution contexts.
Fiber.has_constant?(:ExecutionContext) could be good. But it's a bit fragile. Even if we remove the currently existing definitions, it could easily slip in again (we'd probably notice, but it's still annoying).
Perhaps we should just check flag?(:execution_context)? And eventually migrate to !flag?(:no-mt)?

@ysbaddaden
Copy link
Contributor

Checking flag?(:execution_context) isn't forward safe, so general libraries should check for Fiber.has_constant?(:ExecutionContext) instead (they're unlikely to pull the namespace) but stdlib might not need to bother and we'll just fix the flag when we release the feature, so 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants