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

chore: use Arc instead of lifetime parameter #276

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

mohanson
Copy link
Collaborator

@mohanson mohanson commented Aug 9, 2022

This PR modified the definition of AsmMachine:

// Before
pub struct AsmMachine<'a> {
    pub machine: DefaultMachine<'a, Box<AsmCoreMachine>>,
    pub aot_code: Option<&'a AotCode>,
}

// After
pub struct AsmMachine {
    pub machine: DefaultMachine<Box<AsmCoreMachine>>,
    pub aot_code: Option<Arc<AotCode>>,
}

This allows us to completely remove any lifetime parameter in ckb-vm.

@mohanson mohanson requested a review from xxuejie August 9, 2022 07:26
pub aot_code: Option<&'a AotCode>,
pub struct AsmMachine {
pub machine: DefaultMachine<Box<AsmCoreMachine>>,
pub aot_code: Option<Rc<AotCode>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to use Arc here. It's not that ckb-vm will be using threads, it's more that if you think about the usage patterns, it's likely the same binary being AOTed could be shared amongst multiple ckb-vm instances, all using the same AotCode instance to speed up execution. In this case, AotCode might be shared among multiple threads. Using Rc here severely limits the ability to share compiled immutable code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right.

@mohanson mohanson changed the title chore: use Rc instead of lifetime parameter chore: use Arc instead of lifetime parameter Aug 9, 2022
@mohanson
Copy link
Collaborator Author

mohanson commented Aug 9, 2022

CC @xxuejie

@mohanson mohanson merged commit a92ad61 into nervosnetwork:develop Aug 9, 2022
@mohanson mohanson deleted the remove_lifetime branch August 9, 2022 10:59
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