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

Add expiration slot to loaded program cache entry #31034

Merged
merged 5 commits into from
Apr 5, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Apr 3, 2023

Problem

Need a way to mark cache entry valid for only a certain number of slots. This will be useful to replenish cache with programs using old feature map, while the new programs are being compiled and pre-loaded in the cache. This will also be useful to mark tombstones valid only for certain duration (e.g. for delay visibility feature).

Summary of Changes

  • Add a new optional field to cache entry.
  • Set it for tombstones corresponding to delay visibility feature.
  • Update extract to treat a program as missing if the latest matching cache entry is expired.
  • Update unit test.

Fixes #

@pgarg66 pgarg66 requested a review from Lichtso April 3, 2023 20:51
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #31034 (1fbd74a) into master (e8ea722) will decrease coverage by 0.1%.
The diff coverage is 95.3%.

@@            Coverage Diff            @@
##           master   #31034     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         728      728             
  Lines      205583   205620     +37     
=========================================
- Hits       167711   167670     -41     
- Misses      37872    37950     +78     

@Lichtso
Copy link
Contributor

Lichtso commented Apr 4, 2023

Also prune or evict should clean up expired entries. Probably prune will do in terms of frequency.

@pgarg66
Copy link
Contributor Author

pgarg66 commented Apr 4, 2023

Also prune or evict should clean up expired entries. Probably prune will do in terms of frequency.

Discussed this over chat. This will be combined with rework of eviction code.

@pgarg66 pgarg66 requested a review from Lichtso April 4, 2023 17:19
if working_slot.current_slot() >= entry.effective_slot
&& working_slot.is_ancestor(entry.deployment_slot)
let current_slot = working_slot.current_slot();
if entry
Copy link
Contributor

Choose a reason for hiding this comment

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

Just got thinking if this can be influenced by the fork graph.
What if a fork occurs and one only one fork we have a program upgrade.
Then it places a tombstone, and now that tombstone which has an expiration slot would prevent further lookback on all forks even if they did not upgrade the program.

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, I think we should also check the ancestor relation of the deployment slot with the working slot in this condition. i.e. check for expiration only if the deployment slot is an ancestor of the current slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might not actually solve it. Need more thoughts on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought some more about it. For a program that gets updated on a fork, the tombstone will be created with the corresponding deployment slot. So if we check that the deployment slot of the program is an ancestor of the working slot, it should solve the issue.

@pgarg66 pgarg66 requested a review from Lichtso April 4, 2023 21:26
@pgarg66 pgarg66 merged commit 83e17d8 into solana-labs:master Apr 5, 2023
@pgarg66 pgarg66 deleted the expiration-slot branch April 5, 2023 16:26
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.

2 participants