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

cache executors on failed transactions #22308

Merged

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Jan 5, 2022

Problem

Verified and compiled executors of failed transactions are not cached, causing bursts of failed transactions to continually verify and recompile programs which have an impact on performance

Summary of Changes

Always cached executors whether or not the transaction fails.

Fixes #

@jackcmay jackcmay added the v1.9 label Jan 5, 2022
@jackcmay jackcmay requested a review from t-nelson January 5, 2022 18:55
sdk/src/feature_set.rs Outdated Show resolved Hide resolved
@jackcmay jackcmay force-pushed the cache-executors-on-failed-txs branch from 04601e0 to 5a4ec28 Compare January 5, 2022 20:45
t-nelson
t-nelson previously approved these changes Jan 5, 2022
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.

Nice! r+ ci

@jackcmay jackcmay force-pushed the cache-executors-on-failed-txs branch from 5a4ec28 to b289de2 Compare January 5, 2022 21:40
@mergify mergify bot dismissed t-nelson’s stale review January 5, 2022 21:40

Pull request has been modified.

@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #22308 (24bbea5) into master (6c65734) will decrease coverage by 0.0%.
The diff coverage is 81.5%.

@@            Coverage Diff            @@
##           master   #22308     +/-   ##
=========================================
- Coverage    81.1%    81.0%   -0.1%     
=========================================
  Files         523      551     +28     
  Lines      146793   149616   +2823     
=========================================
+ Hits       119067   121302   +2235     
- Misses      27726    28314    +588     

@jackcmay jackcmay force-pushed the cache-executors-on-failed-txs branch from b289de2 to d4a0097 Compare January 6, 2022 00:05
@jackcmay
Copy link
Contributor Author

jackcmay commented Jan 6, 2022

@jstarry Check this out, this is in response to your comment: #22313 (comment)

@jackcmay jackcmay force-pushed the cache-executors-on-failed-txs branch from d4a0097 to e84bf10 Compare January 6, 2022 00:11
Comment on lines 115 to 117
pub fn miss(&mut self) {
self.is_miss = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

This method is kinda dangerous because it could be used to make an "updated executor" into a cache miss and cause it to be considered dirty even when updates are not included. Can you add a #[cfg(test)] or rename to set_miss_for_tests or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding #[cfg(test)] but that didn't work, maybe that only works locally. I can rename to for_tests

@jstarry
Copy link
Member

jstarry commented Jan 6, 2022

Besides my above comment, looks great! Nice cleanup

@jstarry jstarry added the v1.8 label Jan 6, 2022
@jstarry
Copy link
Member

jstarry commented Jan 6, 2022

Added the v1.8 backport tag, was that left off intentionally?

@jackcmay
Copy link
Contributor Author

jackcmay commented Jan 6, 2022

Added the v1.8 backport tag, was that left off intentionally?

figured it would have to be manually backported to v1.8

@jackcmay jackcmay force-pushed the cache-executors-on-failed-txs branch from e84bf10 to 24bbea5 Compare January 6, 2022 01:29
@jackcmay jackcmay merged commit 12e1602 into solana-labs:master Jan 6, 2022
@jackcmay jackcmay deleted the cache-executors-on-failed-txs branch January 6, 2022 06:09
mergify bot pushed a commit that referenced this pull request Jan 6, 2022
(cherry picked from commit 12e1602)

# Conflicts:
#	program-runtime/src/invoke_context.rs
#	programs/bpf_loader/src/lib.rs
#	runtime/src/bank.rs
mergify bot pushed a commit that referenced this pull request Jan 6, 2022
(cherry picked from commit 12e1602)

# Conflicts:
#	program-runtime/src/invoke_context.rs
#	runtime/src/bank.rs
mergify bot added a commit that referenced this pull request Jan 6, 2022
* cache executors on failed transactions (#22308)

(cherry picked from commit 12e1602)

# Conflicts:
#	program-runtime/src/invoke_context.rs
#	runtime/src/bank.rs

* resolve conflicts

Co-authored-by: Jack May <jack@solana.com>
jackcmay added a commit that referenced this pull request Jan 6, 2022
* cache executors on failed transactions (#22308)

(cherry picked from commit 12e1602)

# Conflicts:
#	program-runtime/src/invoke_context.rs
#	programs/bpf_loader/src/lib.rs
#	runtime/src/bank.rs

* resolve conflicts

Co-authored-by: Jack May <jack@solana.com>
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

3 participants