Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop the gc-sequence crate. #50

Merged
merged 3 commits into from
May 8, 2023
Merged

Drop the gc-sequence crate. #50

merged 3 commits into from
May 8, 2023

Conversation

kyren
Copy link
Owner

@kyren kyren commented May 2, 2023

This has always been a bit of a strange API, and very very hard to use.

The new APIs for transforming Root types in gc-arena and for dynamic roots would probably serve most use cases of this crate. Those APIs also make the make_sequencable_arena! macro unnecessary, as well as having any sort of special "sequencable" arena type, since the root object can easily be changed to contain an in-progress sequence now, making re-implementing the functionality of this crate much easier.

For uses that genuinely need something like Sequence (like maybe luster if I pick it up again), it's probably better to keep a specialized version of that API (which is what I'm going to do if I pick up luster again).

kyren added 3 commits May 2, 2023 01:25
This has always been a bit of a strange API, and very very hard to use.

The new APIs for transforming Root types in `gc-arena` and for dynamic
roots would probably serve *most* use cases of this crate. Those APIs
also make the `make_sequencable_arena!` macro unnecessary, as well as
having any sort of special "sequencable" arena type, since the root
object can easily be changed to contain an in-progress sequence now,
making re-implementing the functionality of this crate much easier.

For uses that genuinely need something like `Sequence` (like maybe
`luster` if I pick it up again), it's probably better to keep a
specialized version of that API (which is what I'm going to do if I pick
up `luster` again).
@kyren
Copy link
Owner Author

kyren commented May 8, 2023

I'm doing it, I'm making it happen.

@kyren kyren merged commit 63dab12 into master May 8, 2023
@kyren kyren deleted the noseq branch May 8, 2023 13:44
@kyren
Copy link
Owner Author

kyren commented May 8, 2023

Just giving some more justification for why I dropped this.

Removing the dependency on gc-sequence dramatically simplified luster (now named 'deimos' because of reasons).

I didn't even bother with combinators.. there is exactly one use case for something like gc-sequence... you have to have all of the following...

  1. A long running operation
  2. That must hold Gc pointers
  3. That cannot hold DynamicRoot handles instead of Gc pointers because the operation state must itself be collectable while it is running, and DynamicRoot handles are opaque to tracing (effectively this would be an uncollectable reference cycle until the operation is complete).
  4. That cannot possibly be written to use the Arena type from the "outside", using Arena::map and the DynamicRootSet
  5. That is very complicated in a way that a combinator API would make it easier, rather than having the calling code just call a polling method over and over on a type that implements Collect.

The ONLY reason for that crate's existence is because luster deimos has this precise set of requirements for long running callbacks, particularly one callback which is coroutine.resume that runs an entire Lua coroutine thread. And then, even there, the requirement that it definitely be collectable would only show up in a specific situation, a dropped coroutine that also contains within it its own Lua thread handle, which you would have to pass into it after creating it.

The thing that sold me on removing this crate was that even after accepting all that justification, the only use case for this entire crate, coroutine.resume, ended up being so simple that having a combinator API makes it more complex rather than less.

The real use for gc-sequence was things that are now covered by things like Arena::map and especially DynamicRootSet. DynamicRootSet's only disadvantage is that it is not visible to tracing, but that is completely fine in the very common intended case of using it from the outside of the arena.

Removing gc-arena from luster deimos dramatically simplified it. If somebody has exactly this use case in a way that the combinator API will be helpful, I will gladly transfer to them ownership of the gc-sequence crate and it can live on, but until then... it's dead, Jim.

This pull request was closed.
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.

1 participant