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

Only keep track of recently used stacks in memory. #2591

Open
wants to merge 33 commits into
base: staging
Choose a base branch
from

Conversation

vicsn
Copy link
Collaborator

@vicsn vicsn commented Jan 10, 2025

Motivation

RSS growth is correlated with deployments. By lazy loading deployments, hopefully unbounded memory growth is eliminated.

The cache will take up at most MAX_PROGRAM_DEPTH x MAX_IMPORTS x 100kb x 10 ~= 4GB of data.

Note that programs, represented as Stacks, have imports. This means the cache is essentially a DAG with multiple roots. To avoid memory leaks, we only evict root stacks from the cache. In the following diagram, you can see in the call graph below where the cache is (temporarily) locked and updated. The "long" loop has some nasty edge cases, it would be better to pass all imports directly into load_deployment and Stack::new, but that would be a much bigger refactor.

Screenshot 2025-01-10 at 16 30 57

Test Plan

  • Unit tests pass. Requires [Refactor] simplify and unify the storage setup for tests #2590 to fix the test_real_example_cache_evict test.
  • Local network passes, using tx-cannon to deploy and fetch multiple sets of maximally nested deployments.
  • Deployed network passes, using tx-cannon to deploy and fetch multiple sets of maximally nested deployments.
  • Consider fixing how test storage works, which currently leaks states across tests.

Related PRs

#2519
#2553
#2578

@vicsn vicsn changed the base branch from staging to mainnet January 10, 2025 16:34
@vicsn vicsn changed the base branch from mainnet to staging January 10, 2025 16:34
This should - as far as we know - eliminate unbounded memory growth.
To avoid memory leaks, we only evict "root" stacks from the cache.
@vicsn vicsn force-pushed the stack_cache_reloaded branch from 6c990e8 to 44fc174 Compare January 10, 2025 17:39
console/network/src/lib.rs Outdated Show resolved Hide resolved
synthesizer/process/src/lib.rs Outdated Show resolved Hide resolved
let credits_id = self
.credits
.as_ref()
.map_or(ProgramID::<N>::from_str("credits.aleo").unwrap(), |stack| *stack.program_id());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.map_or(ProgramID::<N>::from_str("credits.aleo").unwrap(), |stack| *stack.program_id());
.map_or_else(|| ProgramID::<N>::from_str("credits.aleo").unwrap(), |stack| *stack.program_id());

(to avoid eager evaluation)

let programs_to_add = programs_to_add
.into_iter()
.chain(std::iter::once((*stack.program_id(), stack))) // add the root stack.
.unique_by(|(id, _)|*id) // don't add duplicates.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.unique_by(|(id, _)|*id) // don't add duplicates.
.unique_by(|(id, _)| *id) // don't add duplicates.

tiny nit, rustfmt might have missed it

Comment on lines +314 to +319
let mut process = Self {
universal_srs: Arc::new(UniversalSRS::load()?),
credits: None,
stacks: Arc::new(Mutex::new(LruCache::new(NonZeroUsize::new(N::MAX_STACKS).unwrap()))),
store: None,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mut process = Self {
universal_srs: Arc::new(UniversalSRS::load()?),
credits: None,
stacks: Arc::new(Mutex::new(LruCache::new(NonZeroUsize::new(N::MAX_STACKS).unwrap()))),
store: None,
};
let mut process = Self::load_no_storage()?;

Comment on lines +256 to +261
let mut process = Self {
universal_srs: Arc::new(UniversalSRS::load()?),
credits: None,
stacks: Arc::new(Mutex::new(LruCache::new(NonZeroUsize::new(N::MAX_STACKS).unwrap()))),
store: Some(store),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mut process = Self {
universal_srs: Arc::new(UniversalSRS::load()?),
credits: None,
stacks: Arc::new(Mutex::new(LruCache::new(NonZeroUsize::new(N::MAX_STACKS).unwrap()))),
store: Some(store),
};
let mut process = Self::load_no_storage()?;
process.store = Some(store);

Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

2nd review pass complete, left a few comments; also, it seems like one of the new tests is currently failing.

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