Skip to content

Make DynamicRootSet store bare types rather than Gc pointers#54

Merged
kyren merged 4 commits intomasterfrom
better_dyn_roots
May 7, 2023
Merged

Make DynamicRootSet store bare types rather than Gc pointers#54
kyren merged 4 commits intomasterfrom
better_dyn_roots

Conversation

@kyren
Copy link
Copy Markdown
Owner

@kyren kyren commented May 6, 2023

Improves the usability of the DynamicRootSet by a great deal, also reduces the size of DynamicRoot types.

Also makes #53 not strictly necessary anymore.

There is one situation where one might be slightly worse off this way, which is that there will be an extra pointer indirection if the user was already going to store a Gc<'gc, T>, but in all other cases this is a strict improvement.

@kyren kyren force-pushed the better_dyn_roots branch from 07ba7d3 to 44df529 Compare May 6, 2023 04:36
@kyren kyren force-pushed the better_dyn_roots branch from 9dd9601 to 0a49d46 Compare May 6, 2023 11:17
@kyren
Copy link
Copy Markdown
Owner Author

kyren commented May 6, 2023

Added a few general fixes to the dynamic roots stuff as well.

@kyren
Copy link
Copy Markdown
Owner Author

kyren commented May 6, 2023

Talked about this in Discord a bit, but just writing this here so I remember it later...

One thing I didn't think about is that following the held Rc pointer will always happen on access anyway, because we need to check the SetId pointer, then the held object will be right next to it, so it's probably not actually much of a downside? We should probably have performance tests...

If we were concerned about API stability, it would lock us out of a potentially faster API later, but... we could also just add a faster API in addition to this one, since It's still a bit obnoxious to root a non-Gc type with the old API. We could probably make something that didn't require a pointer indirection to check the set ID, and then having the object pointer right there would be a win, but even then there's still going to be pointer indirection to change the Rc count... I'm really not knowledgable enough to know what the exact impact would be.

The main case I see here for this is simplification, I have a big Value type enum with 9 variants, then some of those variants have other variants, some of which contain Gc internally and some contain GcCell internally.

This allows you to make registered types without having to "unfold" enums like this, makes it so i don't have to write a parallel GcCell API, and makes it so that #53 is not required. If we had fine grained mutability and no GcCell type, this would mabe change the equation a bit, but I still really don't like being forced to wrap and unfold enums, and #53 had a bit more splash damage in https://github.com/kyren/deimos than I anticipated.

@kyren kyren merged commit 7148c52 into master May 7, 2023
@kyren kyren deleted the better_dyn_roots branch May 9, 2023 16:44
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