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

removes CowCachedExecutors #22343

Merged

Conversation

behzadnouri
Copy link
Contributor

Problem

Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526

Summary of Changes

  • removed CowCachedExecutors.

Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526
@behzadnouri behzadnouri requested review from jackcmay, Lichtso and t-nelson and removed request for Lichtso, t-nelson and jackcmay January 6, 2022 20:02
@behzadnouri behzadnouri marked this pull request as draft January 6, 2022 20:15
@behzadnouri behzadnouri marked this pull request as ready for review January 6, 2022 20:25
@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #22343 (ede5b4a) into master (f1e2598) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #22343   +/-   ##
=======================================
  Coverage    81.0%    81.0%           
=======================================
  Files         551      551           
  Lines      149806   149776   -30     
=======================================
- Hits       121476   121466   -10     
+ Misses      28330    28310   -20     

@jackcmay
Copy link
Contributor

jackcmay commented Jan 6, 2022

Does this prevent two bank forks from poisoning each other?

@behzadnouri
Copy link
Contributor Author

Does this prevent two bank forks from poisoning each other?

yes, Arc::make_mut will make a clone if there are other Arc pointers to the same allocation:
https://doc.rust-lang.org/std/sync/struct.Arc.html#method.make_mut

@jackcmay
Copy link
Contributor

jackcmay commented Jan 6, 2022

Does this prevent two bank forks from poisoning each other?

yes, Arc::make_mut will make a clone if there are other Arc pointers to the same allocation: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.make_mut

True, yeah, awesome!

Copy link
Contributor

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

Much better

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm! Kinda tangential to the eviction improvements we're trying to fast-track though. Should we hold off until those are in and a release cut?

@behzadnouri
Copy link
Contributor Author

lgtm! Kinda tangential to the eviction improvements we're trying to fast-track though. Should we hold off until those are in and a release cut?

I think it makes the code simpler and might help with follow up improvements.
I will shepherd the backports not to cause any interruptions.

@behzadnouri behzadnouri merged commit c2389fc into solana-labs:master Jan 7, 2022
@behzadnouri behzadnouri deleted the rm-cow-cached-executors branch January 7, 2022 14:06
mergify bot pushed a commit that referenced this pull request Jan 7, 2022
Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526

(cherry picked from commit c2389fc)

# Conflicts:
#	runtime/src/bank.rs
mergify bot pushed a commit that referenced this pull request Jan 7, 2022
Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526

(cherry picked from commit c2389fc)
mergify bot added a commit that referenced this pull request Jan 7, 2022
Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526

(cherry picked from commit c2389fc)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
mergify bot added a commit that referenced this pull request Jan 7, 2022
* removes CowCachedExecutors (#22343)

Copy-on-write semantics for cached executors can be implemented by a
simple Arc<CachedExecutors> as opposed to CowCachedExecutors:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L244-L247

This will also avoid the need for double locking as in:
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3490-L3491
https://github.com/solana-labs/solana/blob/f1e2598ba/runtime/src/bank.rs#L3525-L3526

(cherry picked from commit c2389fc)

# Conflicts:
#	runtime/src/bank.rs

* removes backport merge conflicts

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
@behzadnouri
Copy link
Contributor Author

lgtm! Kinda tangential to the eviction improvements we're trying to fast-track though. Should we hold off until those are in and a release cut?

I think it makes the code simpler and might help with follow up improvements. I will shepherd the backports not to cause any interruptions.

@t-nelson backports now merged in.

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.

None yet

4 participants