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

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wks
Copy link
Collaborator

@wks wks commented Aug 28, 2022

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 wks changed the title Remove the <VM as VMBinding>::Foo::bar bullsh*t. Remove the unpleasant <VM as VMBinding>::Foo::bar code Aug 28, 2022
@wks
Copy link
Collaborator Author

wks commented Aug 28, 2022

In #606 (comment), 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.

@qinsoon
Copy link
Member

qinsoon commented Aug 28, 2022

This is a Rust compiler's problem: rust-lang/rust#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()

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.

2 participants