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

Cleanup - Removes LoadedProgram::maybe_expiration_slot #35023

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Jan 31, 2024

Problem

When switching from explicit to implicit DelayVisibility tombstones, we have forgotten to remove their expiration mechanism which became unreachable:
During extraction the DelayVisibility tombstone is created implicitly and inserted into the batch cache as a hit. Because it does not count as a miss, it is never loaded and assigned / replenished. Thus, the DelayVisibility tombstones only exists on the TX batch level, not on the global loaded programs cache. So, they can never be encountered during extract() and don't need to be skipped. And they can also not be pruned by expiration as they never get inserted in the first place.

Summary of Changes

  • Removes LoadedProgram::maybe_expiration_slot
  • Removes Stats::prunes_expired
  • Removes the logic that skips DelayVisibility tombstones in LoadedPrograms::is_entry_usable()
  • Removes the logic that prunes DelayVisibility tombstones in LoadedPrograms::prune()
  • Removes related tests

@Lichtso Lichtso requested a review from pgarg66 January 31, 2024 16:22
@Lichtso Lichtso force-pushed the cleanup/remove_loaded_program_expiration_slot branch from 2cc45cf to cdaf52c Compare January 31, 2024 16:25
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (fddfc84) 81.6% compared to head (e62a1e8) 81.6%.
Report is 1 commits behind head on master.

❗ Current head e62a1e8 differs from pull request most recent head 6528ce9. Consider uploading reports for the commit 6528ce9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #35023     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         830      830             
  Lines      225031   224844    -187     
=========================================
- Hits       183748   183584    -164     
+ Misses      41283    41260     -23     

pgarg66
pgarg66 previously approved these changes Jan 31, 2024
@Lichtso Lichtso force-pushed the cleanup/remove_loaded_program_expiration_slot branch from cdaf52c to e62a1e8 Compare February 6, 2024 20:49
pgarg66
pgarg66 previously approved these changes Feb 7, 2024
@Lichtso Lichtso force-pushed the cleanup/remove_loaded_program_expiration_slot branch from e62a1e8 to 6528ce9 Compare February 7, 2024 00:13
@Lichtso Lichtso added the automerge Merge this Pull Request automatically once CI passes label Feb 7, 2024
@mergify mergify bot merged commit 070a5a3 into solana-labs:master Feb 7, 2024
36 checks passed
@Lichtso Lichtso deleted the cleanup/remove_loaded_program_expiration_slot branch February 7, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants