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 Trace object-safe, allowing dynamic types and slices to be garbage collected #20

Closed
wants to merge 3 commits into from

Conversation

Techcable
Copy link
Member

@Techcable Techcable commented Jan 18, 2021

The simple part is making a dynamically dispatched version of 'Trace' and 'GcVisitor'. I just added where Self: Sized bounds to all the generic methods and added an object-safe fallback.

This main problem is that 'Trace' and 'GcSafe' have associated
constants 'Trace::NEEDS_TRACE' and 'GcSafe::NEEDS_DROP'.

I've separated these associated consts into their own GcTypeInfno
trait.

I've messed around with this all day.
So far it looks like we might end up requiring significant amounts of
nightly features D:

Fixes #15
Required for #16

Goals

  1. Include 'NEEDS_TRACE' optimization to avoid un-needed tracing
    • Possibly just use NullTrace maker as a simpler alternative
  2. Avoid invoking auto-derived dummy drops
    • These are generated by the derive code, forbidding the user from implementing a custom drop that resurrects objects
    • In that case std::mem::needs_drop may return a false-positive true
  3. Avoid adding new nightly features
    • If we do need to add any they must have an accepted RFC
    • Ideally, we don't want to add very complex features (IE: not specialization)
  4. Preserve static type information and avoid dynamic dispatch wherever possible
    • Can we include some sort of StaticTypeLayout struct in the future
    • This depends on static type information being perserve

Approaches

I see three main approaches that keep the current type hierarchy, and there are pitfalls to each one.

Right now I'm leaning towards separate DynTrace types or having separate trait requirements for sized and unsized types.

Require where *const Self: GcTypeInfo for all Trace types

Directly extending Trace from GcTypeInfo doesn't work. The rust compiler requires supertypes to be object-safe.
When wrapped with a pointer, the type can then implement non-object-safe constants. We simply add functions needs_trace and needs_gc_drop which query <*const T as GcTypeInfo>::NEEDS_TRACE (or NEEDS_DROP).

The big advantage is this approach works on stable!
The disadvantage is that the bound isn't implied for generic parameters.
If you have impl<T: Trace> Trace for Box<T> then the compiler complains that *const T doesn't implement GcTypeInfo.
You must explicitly add a where-bound where *const T: GcTypeInfo to each and every generic parameter 🙄

The implied generic parameters would be fixed by RFC 2089. However, this would probably require nightly, maybe even for API clients.

Specialization

We could use specialization internally to implement needs_trace and needs_drop functions.

trait HiddenTypeInfo {
    const NEEDS_TRACE: bool;
    const NEEDS_DROP: bool;
}
default impl<T: ?Sized> HiddenTypeInfo for T {
    const NEEDS_TRACE: bool = true; // conservatively assume trace
    const NEEDS_DROP: bool = true; // conservative assume drop
}
impl<T: NullTrace> for T {
    const NEEDS_TRACE: bool = false;
}
impl<T: DummyDrop> for T {
    const NEEDS_DROP: bool = false;
}

Problems

  • Unfortunately, this doesn't work with min_specialization
    • The full specialization feature is unsound, so we absolutely want to avoid it
    • It breaks requirement 3
      • I think this has something to do with always applicable impls
      • This could be fixed (in theory) if we used the compiler-internal attribute #[rustc_unsafe_specialization_marker]
    • Maybe we should just ignore it since we know that our specific use-case is fine?
  • How would we require GcTypeInfo to be implemented for all Trace + Sized types?

Associated Type

We could require an associated type type Info: GcTypeInfo. By default, the associated type would be Self for any Sized types. However, for unsized types we would have Info = DefaultTypeInfo that gives conservative defaults.

Associated type defaults were accepted in RFC 2532.

Problems

  • The feature is unstable (though not as bad as specialization).
  • This is only theoretical. I haven't tried it yet
  • Does this work if we don't have a Self: GcTypeInfo bound on the trait?
  • Kind of an ugly approach

Separate erased traits: DynTrace and DynGcVisitor

This is what erased serde did. This is the first approach I tried. It was complex because I had to essentially make the GC accept both Trace and DynTrace. They often needed to be treated separately and there was unessicarry code duplication :(

Maybe we should make the Trace trait object safe by default, but keep GcSafe sized?
The main type that requires GcSafe is Gc<'gc, T> (for safety). That requirement could certainly be relaxed to only the constructors.......

Could we have separate traits GcSafe and GcStaticType? Any sized type must implement GcStaticType and give us the flags (NEEDS_TRACE/NEEDS_DROP) that we want.

This is a problem because 'Trace' and 'GcSafe' have associated
constants 'Trace::NEEDS_TRACE' and 'GcSafe::NEEDS_DROP'.

I've seperated these associatred consts into their own GcTypeInfno
trait.

I've messed around with this all day......
So far it looks like we might end up requiring signficant amounts of
nightly features D:
@Techcable Techcable added this to the 0.2.0 milestone Jan 18, 2021
@Techcable Techcable self-assigned this Jan 18, 2021
@Techcable Techcable marked this pull request as draft January 18, 2021 03:13
This will be required for allocation in a 'Gc',
unless the type is a slice or something.
All other types will just implement trace directly.
I was planning to have three traits (GcType, GcSlice, GcDynObject)
for types which are sized, slices, and unsized respectively.
This wont work out because they are overlapping impls -_-
@Techcable
Copy link
Member Author

This needs to be reimplemented with the new pointer metadata API.

I'm considering just making everything thin pointers so that it is easier for the collector to trace, and more consistent with the .

The most important ?Sized types to trace are slices. I'd really like it if the collector was able to allocate arrays/slices and not just individual objects.

@Techcable
Copy link
Member Author

Superceeded by #29

@Techcable Techcable closed this Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbage collected trace objects (dyn Trace)
1 participant