Refactor - LoadedPrograms::replenish()
#35145
Conversation
Lichtso
force-pushed
the
refactor/loaded_programs_replenish
branch
from
e086bbd
to
89c6c72
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #35145 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 833 833
Lines 224827 224832 +5
=======================================
+ Hits 183523 183545 +22
+ Misses 41304 41287 -17 |
| @@ -712,7 +707,10 @@ impl<FG: ForkGraph> LoadedPrograms<FG> { | |||
| if existing.deployment_slot == entry.deployment_slot | |||
| && existing.effective_slot == entry.effective_slot | |||
| { | |||
| if matches!(existing.program, LoadedProgramType::Unloaded(_)) { | |||
| if matches!( | |||
There was a problem hiding this comment.
instead of this, we should do
match existing.program {
LoadedProgramType::Unloaded(_) | LoadedProgramType::Builtin(_) => {
// The unloaded program is getting reloaded
...
}
LoadedProgramType::FailedVerification(_) => ...,
LoadedProgramType::Closed,
| LoadedProgramType::DelayVisibility
| LoadedProgramType::LegacyV0(_)
... => ...
so we're forced to explicitly express the logic for each variant, which makes it harder to accidentally introduce bugs if the variants change/when the code is updated.
There was a problem hiding this comment.
This PR is to get us in sync with v1.17 again. Other refactorings can come later separately.
Lichtso
force-pushed
the
refactor/loaded_programs_replenish
branch
from
de86842
to
964c410
Compare
Lichtso
force-pushed
the
refactor/loaded_programs_replenish
branch
2 times, most recently
from
7774733
to
87705e1
Compare
There was a problem hiding this comment.
I'm approving this, even though I think that #35145 (comment) needs to be done. For the sake of making backporting easier, since we already have this change in 1.17, it makes sense to do that change separately.
| } | ||
|
|
||
| #[test] | ||
| fn test_assign_program_tombstones() { |
There was a problem hiding this comment.
nit: a comment explaining what this tests would be great
| assert!(cache.replenish(program1, new_test_loaded_program(20, 21)).0); | ||
| cache.assign_program(program1, new_test_loaded_program(0, 1)); | ||
| cache.assign_program(program1, new_test_loaded_program(10, 11)); | ||
| cache.assign_program(program1, new_test_loaded_program(20, 21)); |
There was a problem hiding this comment.
we're not testing what happens when you insert same (deploy, effective) entry anymore?
| }) { | ||
| Ok(index) => { | ||
| let existing = slot_versions.get_mut(index).unwrap(); | ||
| if std::mem::discriminant(&existing.program) |
There was a problem hiding this comment.
I think it would be better to spell out the variants here, and debug_assert on
the invalid combinations. This currently accepts invalid pairs
|
Backports to the beta 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. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
* Replace LoadedPrograms::replenish() with LoadedPrograms::assign_program(). * Removes LoadedPrograms::replenish(). * Defines replacement by having the same loaded program type. * Implements a proper insertion sort with a two key comparison operator. (cherry picked from commit 716ad54)
* Replace LoadedPrograms::replenish() with LoadedPrograms::assign_program(). * Removes LoadedPrograms::replenish(). * Defines replacement by having the same loaded program type. * Implements a proper insertion sort with a two key comparison operator. (cherry picked from commit 716ad54)
* Replace LoadedPrograms::replenish() with LoadedPrograms::assign_program(). * Removes LoadedPrograms::replenish(). * Defines replacement by having the same loaded program type. * Implements a proper insertion sort with a two key comparison operator. (cherry picked from commit 716ad54)
* Replace LoadedPrograms::replenish() with LoadedPrograms::assign_program(). * Removes LoadedPrograms::replenish(). * Defines replacement by having the same loaded program type. * Implements a proper insertion sort with a two key comparison operator. (cherry picked from commit 716ad54)
…35197) Refactor - `LoadedPrograms::replenish()` (#35145) * Replace LoadedPrograms::replenish() with LoadedPrograms::assign_program(). * Removes LoadedPrograms::replenish(). * Defines replacement by having the same loaded program type. * Implements a proper insertion sort with a two key comparison operator. (cherry picked from commit 716ad54) Co-authored-by: Alexander Meißner <[email protected]>
Problem
LoadedPrograms::replenish()was originally implemented to support deduplication of loaded entries when threads raced to load their TX batches. Now, with cooperative loading an entry should never be loaded twice.Summary of Changes
LoadedPrograms::replenish()byLoadedPrograms::assign_program().