Skip to content

Conversation

@qinsoon
Copy link
Member

@qinsoon qinsoon commented Oct 11, 2021

This PR extracts common work packets generation from each plan to a schedule_common() function. Each plan will call this. This closes #368.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Oct 11, 2021
@qinsoon qinsoon mentioned this pull request Oct 18, 2021
@qinsoon qinsoon marked this pull request as ready for review October 27, 2021 05:02
@qinsoon qinsoon requested a review from wks October 27, 2021 05:03
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for now.

But I think we can further improve readability by using associated types.


pub fn schedule_common<E: ProcessEdgesWork<VM = VM>>(
/// Schedule all the common work packets
pub fn schedule_common<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has too many type parameters. May be simplified using traits and associated types.

debug!("Nursery GC");
self.common()
.schedule_common::<GenNurseryProcessEdges<VM, GenImmixCopyContext<VM>>>(
.schedule_common::<Self, GenNurseryProcessEdges<VM, GenImmixCopyContext<VM>>, GenImmixCopyContext<VM>>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type arguments here is especially complicated, with type parameters inside type parameters.

@wks
Copy link
Collaborator

wks commented Oct 27, 2021

Here is my attempt to group together several type parameters using struct and impl. In the example below, the WorkContext trait allows the work function to take only one type parameter instead of three.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6474d424e8c5430e599bc58987c166c3

However, it currently crashes the Rust compiler, probably because the constant generic parameter feature is unstable. Comment out the last two lines in main will make it work.

fn main() {
    work::<StringToScreen>();
//    work::<IntToScreenDec>();
//    work::<IntToScreenHex>();
}

@qinsoon
Copy link
Member Author

qinsoon commented Oct 28, 2021

Here is my attempt to group together several type parameters using struct and impl. In the example below, the WorkContext trait allows the work function to take only one type parameter instead of three.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6474d424e8c5430e599bc58987c166c3

However, it currently crashes the Rust compiler, probably because the constant generic parameter feature is unstable. Comment out the last two lines in main will make it work.

fn main() {
    work::<StringToScreen>();
//    work::<IntToScreenDec>();
//    work::<IntToScreenHex>();
}

I worked on this a bit. We can introduce a new trait:

pub trait GCWorkContext<VM: VMBinding> {
    type PlanType: Plan<VM = VM>;
    type CopyContextType: CopyContext<VM = VM> + GCWorkerLocal;
    type ProcessEdgesWorkType: ProcessEdgesWork<VM = VM>;
}

We can use this for schedule_common(), and it works fine. I feel this type could be useful for other places as well, but I don't have a clear idea where they can be used. I would suggest doing this change in another PR. @wks

@qinsoon qinsoon merged commit bf54dd3 into master Oct 28, 2021
@qinsoon qinsoon deleted the extract-to-schedule-common branch October 28, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proper use of schedule_common()

3 participants