Fiber-local storage #256

SquidDev · 2022-07-19T20:21:50Z

I'm not super happy with the interface for this, so felt it was worth opening an issue and starting a bit of a discussion before spending any more time polishing this off.

Some comments:

I initially went for Context to describe the store of fibre-local variables, sort of mimicking Async's Execution_context and Python's contextvarss library. However, I'm not sure this is the best term - it definitely ends up confusing as eio also has Fiber_context.t.
The store of fibre-local variables is inherited from the currently running fibre when forking (rather than the passed switch). This is (at least in my minds) more intuitive, but does involve an extra Effect.perform Get_context call on each fork, which obviously comes with some performance cost.

talex5

Thanks - this looks good!

I'm not sure about the interface either. I've made a branch experimenting with some naming changes here: https://github.com/ocaml-multicore/eio/compare/main...talex5:eio:fiber-storage?expand=1

I moved the basic get and set functions to Fiber (e.g. Fiber.get key), which avoids the word "context" and maybe makes it more obvious they're attached to fibers. It could do with an introduction explaining what fiber-local variables are for, and maybe discouraging over-use of them.
Renamed context to vars in Fiber_context.
Renamed with_value -> with_binding. We might want a without_binding too.
I removed the convenience Fiber_context.make. I think it's a bit clearer to be explicit here. For example, Fiber.any really only needs to get them once.
I removed the code to inherit across domains for now. I think this is a bit risky, because the values may not be thread-safe. Domain has a split_from_parent argument (https://github.com/ocaml/ocaml/blob/8dda88493c0e8bfec83d3d40129bc6e0fb7cef29/stdlib/domain.mli#L95). Possibly something like that will be useful, but we can add it later.
For now, I removed the API to get and set the whole set at once (only domains needed it). If it comes back, it should probably go in Private.

Do you think this is better?

SquidDev · 2022-07-21T09:15:43Z

I moved the basic get and set functions to Fiber

Oh, that's a fantastic change. That makes things much nicer!

I removed the code to inherit across domains for now. I think this is a bit risky, because the values may not be thread-safe.

For now, I removed the API to get and set the whole set at once (only domains needed it). If it comes back, it should probably go in Private.

Ahh, that's a good point. And agreed - I added this going "weelllll, I can see when this might be useful", but probably worth deferring until there's more concrete use-cases.

Are you happy for me pull in your changes and then polish docs off, etc...?

talex5 · 2022-07-21T09:34:37Z

Are you happy for me pull in your changes and then polish docs off, etc...?

That would be great - thanks!

SquidDev

I've fleshed out the docs a little. I'm afraid I found it much harder to write a succinct introduction to them, and I think what I've ended up with is a possibly a little too sparse.

SquidDev · 2022-07-23T07:58:07Z

lib_eio/core/fiber.ml

@@ -128,7 +132,8 @@ let any fs =
          | [] -> await_cancel ()
          | [f] -> wrap f; []
          | f :: fs ->
-            let new_fiber = Cancel.Fiber_context.make ~cc in
+            let vars = Cancel.Fiber_context.get_vars () in


Did we want to hoist this outside the loop?

Yes please (I was just lazy).

I wasn't sure if it was worth checking if the list was empty here - I'd personally argue not, as any [] should be pretty rare, but happy to handle it if you disagree!

It's fine I think. If we really cared about optimising the empty-list case, we'd do it at the start of the function anyway.

talex5

Thanks, this is looking good :-)

lib_eio/core/eio__core.mli

talex5 · 2022-07-23T08:41:46Z

lib_eio/core/fiber.ml

@@ -128,7 +132,8 @@ let any fs =
          | [] -> await_cancel ()
          | [f] -> wrap f; []
          | f :: fs ->
-            let new_fiber = Cancel.Fiber_context.make ~cc in
+            let vars = Cancel.Fiber_context.get_vars () in


Yes please (I was just lazy).

lib_eio/domain_manager.ml

tests/test_domains.md

lib_eio/core/eio__core.mli

talex5

This PR is still marked as a draft, but it looks ready to merge to me!

talex5 · 2022-07-24T11:31:14Z

lib_eio/core/fiber.ml

@@ -128,7 +132,8 @@ let any fs =
          | [] -> await_cancel ()
          | [f] -> wrap f; []
          | f :: fs ->
-            let new_fiber = Cancel.Fiber_context.make ~cc in
+            let vars = Cancel.Fiber_context.get_vars () in


It's fine I think. If we really cared about optimising the empty-list case, we'd do it at the start of the function anyway.

talex5 · 2022-07-25T09:15:36Z

Thanks!

SquidDev · 2022-07-25T09:29:17Z

Thank you for all your help too! Looking forward to using this :)

@talex5

CHANGES: Note: Eio 0.4 drops compatibility with OCaml 4.12+domains. Use OCaml 5.0.0~alpha1 instead. API changes: - `Eio.Dir` has gone. Use `Eio.Path` instead (@talex5 ocaml-multicore/eio#266 ocaml-multicore/eio#270). - `Eio_unix.FD.{take,peek}` were renamed to `take_opt`/`peek_opt` to make way for non-optional versions. New features: - Fiber-local storage (@SquidDev ocaml-multicore/eio#256). Attach key/value bindings to fibers. These are inherited across forks. - `Eio.Path.{unlink,rmdir,rename}` (@talex5 ocaml-multicore/eio#264 ocaml-multicore/eio#265). - `Eio_main.run` can now return a value (@talex5 ocaml-multicore/eio#263). This is useful for e.g. cmdliner. - `Eio_unix.socketpair` (@talex5 ocaml-multicore/eio#260). - `Fiber.fork_daemon` (@talex5 ocaml-multicore/eio#252). Create a helper fiber that does not prevent the switch from exiting. - Add `Fiber.{iter,map,filter,fiter_map}` (@talex5 ocaml-multicore/eio#248 ocaml-multicore/eio#250). These are concurrent versions of the corresponding operations in `List`. Bug fixes: - Fix scheduling fairness in luv backend (@talex5 ocaml-multicore/eio#269). - Implement remaining shutdown commands for luv (@talex5 ocaml-multicore/eio#268). - Fix IPv6 support with uring backend (@haesbaert ocaml-multicore/eio#261 ocaml-multicore/eio#262). - Use `Eio.Net.Connection_reset` exception in more places (@talex5 ocaml-multicore/eio#257). - Report use of closed FDs better (@talex5 ocaml-multicore/eio#255). Using a closed FD could previously cause the whole event loop to exit. - Some fixes for cancellation (@talex5 ocaml-multicore/eio#254). - Ensure `Buf_write` still flushes if an exception is raised (@talex5 ocaml-multicore/eio#246). - Do not allow close on `accept_fork` socket (@talex5 ocaml-multicore/eio#245). Documentation: - Document integrations with Unix, Lwt and Async (@talex5 ocaml-multicore/eio#247). - Add a Dockerfile for easy testing (@talex5 ocaml-multicore/eio#224).

@talex5

CHANGES: Note: Eio 0.4 drops compatibility with OCaml 4.12+domains. Use OCaml 5.0.0~alpha1 instead. API changes: - `Eio.Dir` has gone. Use `Eio.Path` instead (@talex5 ocaml-multicore/eio#266 ocaml-multicore/eio#270). - `Eio_unix.FD.{take,peek}` were renamed to `take_opt`/`peek_opt` to make way for non-optional versions. New features: - Fiber-local storage (@SquidDev ocaml-multicore/eio#256). Attach key/value bindings to fibers. These are inherited across forks. - `Eio.Path.{unlink,rmdir,rename}` (@talex5 ocaml-multicore/eio#264 ocaml-multicore/eio#265). - `Eio_main.run` can now return a value (@talex5 ocaml-multicore/eio#263). This is useful for e.g. cmdliner. - `Eio_unix.socketpair` (@talex5 ocaml-multicore/eio#260). - `Fiber.fork_daemon` (@talex5 ocaml-multicore/eio#252). Create a helper fiber that does not prevent the switch from exiting. - Add `Fiber.{iter,map,filter,fiter_map}` (@talex5 ocaml-multicore/eio#248 ocaml-multicore/eio#250). These are concurrent versions of the corresponding operations in `List`. Bug fixes: - Fix scheduling fairness in luv backend (@talex5 ocaml-multicore/eio#269). - Implement remaining shutdown commands for luv (@talex5 ocaml-multicore/eio#268). - Fix IPv6 support with uring backend (@haesbaert ocaml-multicore/eio#261 ocaml-multicore/eio#262). - Use `Eio.Net.Connection_reset` exception in more places (@talex5 ocaml-multicore/eio#257). - Report use of closed FDs better (@talex5 ocaml-multicore/eio#255). Using a closed FD could previously cause the whole event loop to exit. - Some fixes for cancellation (@talex5 ocaml-multicore/eio#254). - Ensure `Buf_write` still flushes if an exception is raised (@talex5 ocaml-multicore/eio#246). - Do not allow close on `accept_fork` socket (@talex5 ocaml-multicore/eio#245). Documentation: - Document integrations with Unix, Lwt and Async (@talex5 ocaml-multicore/eio#247). - Add a Dockerfile for easy testing (@talex5 ocaml-multicore/eio#224).

SquidDev marked this pull request as draft July 19, 2022 20:21

Draft implementation of fiber-local storage

afab28c

SquidDev force-pushed the feature/fiber-storage branch from a787f98 to afab28c Compare July 20, 2022 09:35

Testing some API changes

54d64d5

talex5 reviewed Jul 21, 2022

View reviewed changes

SquidDev added 2 commits July 23, 2022 08:55

Add without_binding and some docs

e79f758

Clarify that FLS isn't always a good idea

222136c

SquidDev commented Jul 23, 2022

View reviewed changes

talex5 reviewed Jul 23, 2022

View reviewed changes

Some further doc cleanup

05d5bc8

talex5 approved these changes Jul 24, 2022

View reviewed changes

SquidDev marked this pull request as ready for review July 24, 2022 11:36

Move fiber-local storage tests to fiber test suite

c166b06

talex5 merged commit 8587998 into ocaml-multicore:main Jul 25, 2022

SquidDev deleted the feature/fiber-storage branch July 25, 2022 09:22

SquidDev mentioned this pull request Jul 25, 2022

Fibre-local storage #253

Closed

talex5 mentioned this pull request Aug 4, 2022

[new release] eio_main, eio_luv, eio_linux and eio (0.4) ocaml/opam-repository#21935

Merged

ELLIOTTCABLE mentioned this pull request Aug 30, 2023

Extract thread_local into separate library, abstract for lwt/eio-friendly continuation-local storage imandra-ai/ocaml-opentelemetry#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fiber-local storage #256

Fiber-local storage #256

SquidDev commented Jul 19, 2022

talex5 left a comment

SquidDev commented Jul 21, 2022

talex5 commented Jul 21, 2022

SquidDev left a comment

SquidDev Jul 23, 2022

talex5 Jul 23, 2022

SquidDev Jul 23, 2022

talex5 Jul 24, 2022

talex5 left a comment

talex5 Jul 23, 2022

talex5 left a comment

talex5 Jul 24, 2022

talex5 commented Jul 25, 2022

SquidDev commented Jul 25, 2022

Fiber-local storage #256

Fiber-local storage #256

Conversation

SquidDev commented Jul 19, 2022

talex5 left a comment

Choose a reason for hiding this comment

SquidDev commented Jul 21, 2022

talex5 commented Jul 21, 2022

SquidDev left a comment

Choose a reason for hiding this comment

SquidDev Jul 23, 2022

Choose a reason for hiding this comment

talex5 Jul 23, 2022

Choose a reason for hiding this comment

SquidDev Jul 23, 2022

Choose a reason for hiding this comment

talex5 Jul 24, 2022

Choose a reason for hiding this comment

talex5 left a comment

Choose a reason for hiding this comment

talex5 Jul 23, 2022

Choose a reason for hiding this comment

talex5 left a comment

Choose a reason for hiding this comment

talex5 Jul 24, 2022

Choose a reason for hiding this comment

talex5 commented Jul 25, 2022

SquidDev commented Jul 25, 2022