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

Make Arena conditionally Send if the root type is Send #61

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

kyren
Copy link
Owner

@kyren kyren commented May 12, 2023

Also makes Gc<'static, T> Send, so that the Arena has a possibility of being send.

I am not totally sure on the soundness of this. It doesn't give any extra powers to any types other than the Arena type and Gc<'static, T>.

It shouldn't be possible to get into trouble with Gc at all, since you can't obtain a Gc<'static> in the first place (and even if you could safely, it could never be freed so that's fine it is totally not fine, and you totally could get a Gc<'static> which could make a !Sync type into a Sync one and I had to change the mutate signatures to prevent it).

You shouldn't be able to get in trouble with Arena, because it is always !Sync, and you won't be able to store anything !Send (other than Gc) inside of it.

A question I have is if there's some way to get in trouble with the projection of the root type, by applying the lifetime to a different type, to get a thing that mentions a type which is only Send if it is 'static (similar to Gc here), and have that be transmuted to 'static, but that can't be safe to do anyway since it would have to be an unsound lifetime transmute, since the 'gc is generative and can't be safely related to whatever type is being projected... I think?

Making Send Arenas would be heckin useful to me, because in ECS design, !Send types are generally always low level painful.

Anyway this is here to be discussed, feel free to crush my hopes and dreams.

@kyren
Copy link
Owner Author

kyren commented May 12, 2023

This turned out to be a whole thing in the presence of dyn Trait types, which are somewhat unergonomic to make + Send. I'm sure it's possible to do via an unsized_send macro as discussed on Discord, but I'm too tired to figure it out right now.

I have cooled somewhat on the idea after seeing some of the implications of it, so I'm just going to leave this for now. I might revisit this idea if it becomes crucial to me, but right now it's more of a pain than pinning to a thread.

@kyren kyren force-pushed the tosendornottosend branch 2 times, most recently from 4e73dac to 3c41c8c Compare May 13, 2023 04:23
@kyren
Copy link
Owner Author

kyren commented May 13, 2023

This turned out to be a whole thing in the presence of dyn Trait types, which are somewhat unergonomic to make + Send. I'm sure it's possible to do via an unsized_send macro as discussed on Discord, but I'm too tired to figure it out right now.

I was no longer too tired to figure it out, it's not that bad at all, PR is revived as it was, hype back to max.

@kyren kyren force-pushed the tosendornottosend branch 3 times, most recently from 0f9cd30 to 09e1598 Compare May 13, 2023 06:05
@kyren
Copy link
Owner Author

kyren commented May 13, 2023

After mulling it over, I'm much more confident that this is sound.

Treating 'gc as 'static must be sound for the same reasoning that underpins the existence of the entire crate.

The only way to access the inside of an arena is to provide a callback that must work for all lifetimes 'gc. Since that callback works for all lifetimes, it must work for the 'static lifetime. The safety of the garbage collector in general relies not on the 'gc lifetime being some unique lifetime, but rather that the callback cannot know what lifetime it is.

The only things directly produced that have the 'gc lifetime are inside the callback and are from gc-arena provided functions. gc-arena can do this, and allow them to be stored in the root object, because gc-arena knows that the 'gc is really 'static.

The safety of the garbage collector derives from the lack of information of the callback, not any specific lifetime. It cannot know what the 'gc lifetime really is, so it must assume it could be non-'static, as well as not allowing it to be unified with any other lifetime (since the provided 'gc is also forced to be invariant). This is what prevents Gc pointers from escaping the arena, being put into thread local storage, being put in a mismatched arena etc, and also with this PR what prevents code from assuming that they are Send.

Being able to project the 'gc lifetime to 'static within unsafe gc-arena code must not cause unsoundness in and of itself, because it is really always 'static, the callback is just not privy to this information. As long as no user code in the callback or outside the arena can witness a 'gc lifetime being 'static, it cannot use this truth to do something unsound, like send Gc pointers to another thread or store them anywhere.

So, I think this PR is sound, at least in that way.. treating 'gc as 'static within gc-arena code is fine, and is actually what all of gc-arena already does. As long as no safe code can ever observe that 'gc is really 'static, we are okay, but that is the foundation of the entire crate.

I feel like I'm repeating myself, but anyway I'm more confident about all this now, and this was the one thing that I was really unsure about. Anyway, I hope all this makes sense, but if it doesn't please tell me!

@kyren
Copy link
Owner Author

kyren commented May 13, 2023

WELL, being sound doesn't make it easy to use...

The extra macro mode that I had worked in very simple cases but immediately fell over in complex ones, I'm still not 100% sure of all the reasons, but it has to do with the fact that you need to implement traits (which may be parameterized over 'gc) for the 'static projection, not the 'gc one, because that's when the pointer cast happens. This is basically unavoidable, because you need the cast to happen with something that points to a Send type and something that implements the trait you want to cast to, and the ONLY way that can happen is if the trait is implemented when projecting to 'static. It is frustrating that we can't somehow prove to rust that the original lifetime is 'static using unsafe, but I couldn't figure out a way.

Instead, I'm going to try an alternate approach which is not really any worse: an EnsureSend wrapper type!

It's definitely easier to create than using the macro, and it's a slightly worse type signature where it is declared.

kyren added 13 commits May 14, 2023 02:14
Also makes `Gc<'static, T>` Send, so that the `Arena` has a possibility
of being send.
...that prevents obtaining a `Gc<'static>` by leaking the arena.

Allowing this unfortunately breaks the safety of making the arena Send,
because you can get a Send `Gc<'static, T>` for some T that is !Sync.

This change fixes this soundness hole, but double unfortunately breaks
the lifetime trick Ruffle uses to not have to carry three lifetimes
around everywhere in its context type.

Still investigating how to resolve these two things in a satisfactory
way...
@kyren kyren force-pushed the tosendornottosend branch from 249d5b2 to 7504e1b Compare May 14, 2023 06:16
@kyren kyren force-pushed the tosendornottosend branch from c1b7109 to ba469bc Compare May 14, 2023 07:53
@kyren
Copy link
Owner Author

kyren commented May 14, 2023

I think EnsureSend is not sound, even though I'm having trouble coming up with a counter example.

Other than EnsureSend or the unsize macro, the logic seems solid, but everything around dyn casts is becoming awful.

I think EnsureSend would totally be sound IF EnsureSend had a Rootable type parameter. This is not that unreasonable of an idea, and I tried it, but then the problem loops entirely around and we can't do proper unsize casting anymore. You might be able to get the unsize casting back with a macro for dyn casting between projections, but... I think we're back to a custom GcSend pointer type.

I can't stand to think about this anymore and I think I'm going to drop this idea for the moment, or at least pause on it and let it stew for a while.

@kyren
Copy link
Owner Author

kyren commented May 14, 2023

Yet again, I failed to drop it.

I am back on the side of thinking EnsureSend is sound, but not for the reasons I gave (instead, for simpler reasons). I have changed the struct name and explanatory comment to reflect my new, simpler logic.

@kyren
Copy link
Owner Author

kyren commented May 14, 2023

I thought about it a lot more and decided that making !Send arenas safe through thread pinning is actually less work than maintaining this API, and I don't really lose much by doing so.

Having Send-able arenas is still a nice ergonomic thing though, but now that I understand that both sides are mostly just questions of ergonomics, it's something I can actually evaluate.

For now, I don't think this is worth it, but I'm not gonna close the PR or anything, just leave it in its current working state until such a time that somebody feels it is worth finishing.

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