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 %}

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