Skip to content

Improve Fiber::ExecutionContext.default_workers_count#16149

Merged
straight-shoota merged 3 commits intocrystal-lang:masterfrom
ysbaddaden:fix/ec-dont-cap-default-workers-count
Sep 21, 2025
Merged

Improve Fiber::ExecutionContext.default_workers_count#16149
straight-shoota merged 3 commits intocrystal-lang:masterfrom
ysbaddaden:fix/ec-dont-cap-default-workers-count

Conversation

@ysbaddaden
Copy link
Collaborator

@ysbaddaden ysbaddaden commented Sep 9, 2025

  • Makes sure there is at least 1 worker (minimum required).
  • Doesn't cap the total to an arbitrary number.
  • Caps the total to the effective logical CPUs when supported.

Depends on #16148.

@yxhuvud
Copy link
Contributor

yxhuvud commented Sep 9, 2025

Caps the total to the effective logical CPUs when supported (linux).

This I feel would be a mistake. While from a pure processing point of view it would probably be ok, there are other situations where you do outgoing calls in some way that may block execution but might not be very costly from a CPU perspective. The classical example would be a thread pool having a bunch of threads reading from spinning disks in parallel - then it may make sense to overcommit by a lot. I suppose the usefulness of that particular example goes down when using io_uring as the event loop, but there are other situations where it may make sense.

EDIT: Oh, this caps only the DEFAULT value? Ok, that make a lot of sense. Never mind then

@ysbaddaden
Copy link
Collaborator Author

It's not even the actual default parallelism, it's the one we recommend when you opt in to MT by resizing the default context.

- Makes sure there is at least 1 worker (minimum required).
- Doesn't cap the total to an arbitrary number.
- Caps the total to the effective logical CPUs when supported (linux).
@ysbaddaden ysbaddaden force-pushed the fix/ec-dont-cap-default-workers-count branch from 70ddc98 to f619d2f Compare September 11, 2025 11:36
@straight-shoota straight-shoota added this to the 1.18.0 milestone Sep 19, 2025
@straight-shoota straight-shoota merged commit 0be6ee6 into crystal-lang:master Sep 21, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Multi-threading Sep 21, 2025
@ysbaddaden ysbaddaden deleted the fix/ec-dont-cap-default-workers-count branch September 21, 2025 16:40
Comment on lines +101 to +102
total = System.cpu_count.to_i.clamp(1..)
effective = Crystal::System.effective_cpu_count.to_i.clamp(1..)
Copy link
Contributor

@Sija Sija Oct 17, 2025

Choose a reason for hiding this comment

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

Aren't #to_i calls redundant (since .cpu_count and .effective_cpu_count methods already return an Int)?

Suggested change
total = System.cpu_count.to_i.clamp(1..)
effective = Crystal::System.effective_cpu_count.to_i.clamp(1..)
total = System.cpu_count.clamp(1..)
effective = Crystal::System.effective_cpu_count.clamp(1..)

Copy link
Collaborator Author

@ysbaddaden ysbaddaden Oct 17, 2025

Choose a reason for hiding this comment

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

No. They may return any Int. For example on Linux it's an u64 or i64, but EC expects an i32.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation 🙏

Comment on lines +101 to +105
total = System.cpu_count.to_i.clamp(1..)
effective = Crystal::System.effective_cpu_count.to_i.clamp(1..)
# TODO: query for CPU limits (e.g. linux/cgroup, freebsd/rctl, ...)

Math.min(total, effective)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ysbaddaden IIUC if either one of the total or effective is not supported by the OS, the corresponding method call will return -1, which will turn into 1 following the #clamp(1..) call, which then in turn will limit the overall count and method's return value to 1, via Math.min(x, 1).

Is that an expected behavior? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uuurgh, you're right, -1 shall skip the value 😭

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

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants