mmtk-core icon indicating copy to clipboard operation
mmtk-core copied to clipboard

Remove the unpleasant <VM as VMBinding>::Foo::bar code

Open wks opened this issue 3 years ago • 2 comments

A VM binding has many aspects, and we represent them as several different traits: ObjectModel, Scanning, Collection, ActivePlan and ReferenceGlue.

Currently, we combine them into one VMBinding by making them member types of the VMBinding trait, namely:

trait VMBinding {
    type VMObjectModel: ObjectModel<Self>;
    type VMScanning: Scanning<Self>;
    type VMCollection: Collection<Self>;
    type VMActivePlan: ActivePlan<Self>;
    type VMReferenceGlue: ReferenceGlue<Self>;
    // ...
}

This has some drawbacks.

One obvious drawback is that sometimes we have write <VM as VMBinding>::SomeType::some_function. This is ugly, because the Rust complier is sometimes not smart enough to figure out VM implements VMBinding, and SomeType is a member of VMBinding.

To solve this problem, we make those traits "trait bounds" of the VMBinding trait. Namely:

pub trait VMBinding:
    ObjectModel<VM = Self>
    + Scanning<VM = Self>
    + Collection<VM = Self>
    + ActivePlan<VM = Self>
    + ReferenceGlue<VM = Self>
{
    // members go here.
}

By doing this, A binding instance (such as OpenJDK) directly implements ObjectModel, Scanning, Collection, ..., instead of aggregating them into member types. So instead of <VM as VMBinding>::Foo::bar, we only need to write VM::bar.

  • Instead of VM::VMObjectModel::copy, we write VM::copy
  • Instead of <VM as VMBinding>::VMCollection::resume_mutators, we write VM::resume_mutators
  • Instead of <E::VM as VMBinding>::VMScanning::scan_thread_roots, we write E::VM::scan_thread_roots
  • Instead of <<E as ProcessEdgesWork>::VM as VMBinding>::VMCollection::schedule_finalization, we write E::VM::schedule_finalization.

wks avatar Aug 28 '22 07:08 wks

In https://github.com/mmtk/mmtk-core/pull/606#discussion_r955626986, Wenyu suggested moving VMEdge to a different trait. But doing so will make it hard to access for the same reason mentioned here. With this change, it is easier to add member types in other sub-traits of VMBinding, and still make them easy to access.

wks avatar Aug 28 '22 07:08 wks

This is a Rust compiler's problem: https://github.com/rust-lang/rust/issues/38078. Also, according to your change list, it seems only 1/4 of our use cases are affected by the problem (where we have to use fully qualified trait names with as), and we can use VM::VMxxxxxx::method() in most cases.

Nonetheless I think this change is okay. One thing we should think about is with the change, methods in the traits are accessed without the trait names, and some of the method names could be a bit ambiguous. These are some examples:

  • VM::VMObjectModel::get_current_size() vs VM::get_current_size()
  • VM::VMObjectModel::copy() vs VM::copy()
  • VM::VMReferenceGlue::get_referent() vs VM::get_referent()

qinsoon avatar Aug 28 '22 22:08 qinsoon