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

Set a global fork graph in program cache #33776

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Oct 19, 2023

Problem

LoadedPrograms::prune() is passed an instance of fork graph. Same information can be used in few other methods in the cache. It'll be ideal if the fork_graph can be set globally for the cache.

Summary of Changes

Set fork_graph at startup and use it in prune.

Fixes #

@pgarg66 pgarg66 force-pushed the global-fork-graph branch 3 times, most recently from aff744c to f6f234c Compare October 20, 2023 02:35
@pgarg66 pgarg66 marked this pull request as ready for review October 20, 2023 03:36
@pgarg66 pgarg66 requested a review from Lichtso October 20, 2023 03:36
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #33776 (8a44175) into master (7aa0fae) will decrease coverage by 0.1%.
Report is 15 commits behind head on master.
The diff coverage is 92.1%.

@@            Coverage Diff            @@
##           master   #33776     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         806      806             
  Lines      217937   218009     +72     
=========================================
+ Hits       178322   178378     +56     
- Misses      39615    39631     +16     

program-runtime/src/loaded_programs.rs Outdated Show resolved Hide resolved
@@ -55,6 +55,7 @@ struct SetRootTimings {
prune_remove_ms: i64,
}

#[derive(Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this can be removed again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it causes the following error

error[E0277]: `BankForks` doesn't implement `std::fmt::Debug`
   --> runtime/src/bank.rs:823:5
    |
661 | #[derive(AbiExample, Debug)]
    |                      ----- in this derive macro expansion
...
823 |     pub loaded_programs_cache: Arc<RwLock<LoadedPrograms<BankForks>>>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `BankForks` cannot be formatted using `{:?}`

Copy link
Contributor

Choose a reason for hiding this comment

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

Then manually: impl Debug for LoadedPrograms {}. Should do that anyway because it contains a lot of things that are unhelpful to display.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agree. Would be better as a separate PR.

@pgarg66 pgarg66 merged commit 59cb3b5 into solana-labs:master Oct 20, 2023
16 checks passed
@pgarg66 pgarg66 deleted the global-fork-graph branch October 20, 2023 15:47
@Lichtso Lichtso added the v1.17 PRs that should be backported to v1.17 label Oct 21, 2023
mergify bot pushed a commit that referenced this pull request Oct 21, 2023
* Set a global fork graph in program cache

* fix deadlock

* review feedback

(cherry picked from commit 59cb3b5)
pgarg66 added a commit that referenced this pull request Oct 21, 2023
…33809)

Set a global fork graph in program cache (#33776)

* Set a global fork graph in program cache

* fix deadlock

* review feedback

(cherry picked from commit 59cb3b5)

Co-authored-by: Pankaj Garg <pankaj@solana.com>
@@ -455,6 +455,20 @@ pub struct LoadedPrograms {
/// Environments of the current epoch
pub environments: ProgramRuntimeEnvironments,
pub stats: Stats,
fork_graph: Option<Arc<RwLock<FG>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should to be a Weak reference.

Choose a reason for hiding this comment

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

Funny that I came across the same problem now: anza-xyz#1893

Copy link
Contributor

mergify bot commented Feb 2, 2024

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants