Skip to content

Add Eio.Fiber.List.partition_map - #927

Open
kentookura wants to merge 1 commit into
ocaml-multicore:mainfrom
kentookura:partition-map
Open

Add Eio.Fiber.List.partition_map#927
kentookura wants to merge 1 commit into
ocaml-multicore:mainfrom
kentookura:partition-map

Conversation

@kentookura

@kentookura kentookura commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

I could not implement this myself since Limiter is hidden from the interface.

Comment thread lib_eio/core/fiber.ml Outdated
| x :: xs ->
let x = Limiter.fork_promise_exn limiter fn x in
let left, right =
match Promise.await x with

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to a fork a promise and then immediately wait on it. Are you sure it's actually running concurrently? It might just be simpler to have a one-liner let partition_map ?max_fibers fn items = List.partition_map Fun.id (map ?max_fibers fn items) (which allocates a bit more, but the dominant cost here would be fiber creation I expect).

@talex5 talex5 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are you sure it's actually running concurrently?

Looks wrong to me too. And the test output also shows that it's not running concurrently.

It might just be simpler to have a one-liner

I agree: it makes sense to start with the one-liner, and optimise it later if it's a problem. @avsm's suggested one-liner also fixes the test output.

@kentookura kentookura closed this Aug 30, 2026
@talex5

talex5 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

(I'm assuming this PR was closed because you decided it's easier to put the one-liner in your own code. Updating this PR to use it would also be fine.)

@kentookura

Copy link
Copy Markdown
Contributor Author

Will do so once I get the chance. Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants