Fix - Reloading of unload entries after finishing the recompilation phase #1199
Conversation
| ) || !Arc::ptr_eq( | ||
| &environments.program_runtime_v2, | ||
| &program_cache.environments.program_runtime_v2, | ||
| ) { |
There was a problem hiding this comment.
This branch was only taken during a feature set change which affects RBPF. Thus, removing it has no affect on the normal operation outside the epoch boundary recompilation phase.
| fn is_current_env( | ||
| environments: &ProgramRuntimeEnvironments, | ||
| env_opt: Option<&ProgramRuntimeEnvironment>, | ||
| ) -> bool { |
There was a problem hiding this comment.
This function always returns true during normal operation outside the epoch boundary recompilation phase. That is because all entries which have an environment have the current environment and all others default to true directly.
There was a problem hiding this comment.
It'll be good to add the above as a comment in the code. It's a subtle condition, which will hard to understand a few months from now.
| let slot_versions = &mut self.entries.entry(key).or_default().slot_versions; | ||
| match slot_versions.binary_search_by(|at| { | ||
| at.effective_slot | ||
| .cmp(&entry.effective_slot) | ||
| .then(at.deployment_slot.cmp(&entry.deployment_slot)) | ||
| .then( |
There was a problem hiding this comment.
This additional then() has no affect on the normal operation outside the epoch boundary recompilation phase. is_current_env() will return true, so this evaluates to .then(true.cmp(&true)) which is .then(std::cmp::Ordering::Equal) which is the same as no .then() statement as it was before.
There was a problem hiding this comment.
Same here, need some comment in the code that explains this.
|
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. |
Lichtso
force-pushed
the
fix/unload_reload_after_recompilation_phase
branch
from
1ffc298
to
407cb83
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1199 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 886 886
Lines 236372 236391 +19
=======================================
+ Hits 194204 194246 +42
+ Misses 42168 42145 -23 |
Lichtso
force-pushed
the
fix/unload_reload_after_recompilation_phase
branch
from
407cb83
to
a810db2
Compare
…hase (#1199) * Adds reproducer to test_feature_activation_loaded_programs_epoch_transition. * Differentiate entries by environment instead of adjusting the effective slot. * Fixes tests of assign_program(). * Fixes env order in test_feature_activation_loaded_programs_recompilation_phase(). * Turns test_load_program_effective_slot() into test_load_program_environment(). * Adds comments inside ProgramCache::assign_program(). (cherry picked from commit 979f619) # Conflicts: # program-runtime/src/loaded_programs.rs # runtime/src/bank/tests.rs # svm/src/program_loader.rs
…hase (#1199) * Adds reproducer to test_feature_activation_loaded_programs_epoch_transition. * Differentiate entries by environment instead of adjusting the effective slot. * Fixes env order in test_feature_activation_loaded_programs_recompilation_phase(). * Adds comments inside ProgramCache::assign_program(). (cherry picked from commit 979f619)
…ation phase (backport of #1199) (#1275) * Fix - Reloading of unload entries after finishing the recompilation phase (#1199) * Differentiate entries by environment instead of adjusting the effective slot. * Adds comments inside ProgramCache::assign_program(). * Adds reproducer to test_feature_activation_loaded_programs_epoch_transition. * Fixes env order in test_feature_activation_loaded_programs_recompilation_phase(). * Syncs both tests with master. --------- (cherry picked from commit 979f619) Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>
…ation phase (backport of anza-xyz#1199) (anza-xyz#1275) * Fix - Reloading of unload entries after finishing the recompilation phase (anza-xyz#1199) * Differentiate entries by environment instead of adjusting the effective slot. * Adds comments inside ProgramCache::assign_program(). * Adds reproducer to test_feature_activation_loaded_programs_epoch_transition. * Fixes env order in test_feature_activation_loaded_programs_recompilation_phase(). * Syncs both tests with master. --------- (cherry picked from commit 979f619) Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>
Problem
Currently the epoch boundary recompilation phase does adjust the effective slot of recompiled entries to be the first slot of the next epoch. However, when reloading unloaded entries, their new loaded entry has an unadjusted effective slot, which will not be matched in
assign_program().Summary of Changes
get_environment()instead of adjusting theeffective_slot.assign_program()to always use the unifedget_mock_env()instead of creating random environments withArc::new(BuiltinProgram::new_mock()).test_feature_activation_loaded_programs_recompilation_phase().test_load_program_effective_slot()intotest_load_program_environment().