SVM: remove usage of program cache from load_program_with_pubkey()

[pgarg66]

Problem

Use of ProgramCache in load_program_with_pubkey() can be removed. It's mainly used to get the instance of RuntimeEnvironments. That can instead be supplied by the caller. Relaxing this dependency will reduce call sites outsize SVM where ProgramCache is being referenced.

Summary of Changes

• Add accessor in TransactionBatchProcessor to get the RuntimeEnvironments for a given epoch
• Pass the environments to load_program_with_pubkey(), removing the need for ProgramCache
• Update unit tests

Fixes #

[LucasSte]

I think you can move the clone directly to ProgramCache get_environmens_for_epoch directly. It is only used in two places and all of them need a clone.

[pgarg66]

Good advice. I'll update the PR.

[LucasSte]

I think this argument does not need to be a reference. Inside this function's match arm, we clone the environments. I believe that if we move this parameter, we don't need to clone the environments, moving them instead.

PS: When I say move, please think about moving memory as opposed to Rust's cloning.

[pgarg66]

Removing the reference won't help much with removing the clone(). The unwrap_or_else() uses the environments and requires prior use of environments to be cloned.

I think keeping the reference here is better. Current users of load_program_with_pubkey() are ok with move as the environments is not referenced at the call site after calling this function. But, this is a public function. Other users may require to hold on to the environments, and it would force them to clone() it before calling this function.

[buffalojoec]

Sorry guys, but I'm actually a little confused as to why we need the clone at all in either method.

As far as I can tell, the argument is a reference - &ProgramRuntimeEnvironments - in every function signature called by the lines changed in this PR.

In my opinion, let both the SVM's and the cache's get_environments_for_epoch return a reference, and clone only where ownership is explicitly required. This seems to be all within tests, except for program cache's new_from_cache, which is explicitly intentional.

[pgarg66]

Sorry guys, but I'm actually a little confused as to why we need the clone at all in either method.

As far as I can tell, the argument is a reference - &ProgramRuntimeEnvironments - in every function signature called by the lines changed in this PR.

In my opinion, let both the SVM's and the cache's get_environments_for_epoch return a reference, and clone only where ownership is explicitly required. This seems to be all within tests, except for program cache's new_from_cache, which is explicitly intentional.

SVM's get_environments_for_epoch() cannot return a reference due to lifetime issues. The read lock of program cache gets dropped when the function returns, and causes the lifetime of inner reference to go out of scope.

The structure carries two Arc<>. So clone of struct is not expensive.

[buffalojoec]

SVM's get_environments_for_epoch() cannot return a reference due to lifetime issues. The read lock of program cache gets dropped when the function returns, and causes the lifetime of inner reference to go out of scope.

The structure carries two Arc<>. So clone of struct is not expensive.

Ahh, okay this was what I was missing. I still prefer the clone at the SVM-level, but I'll leave up to you two.

[pgarg66]

SVM's get_environments_for_epoch() cannot return a reference due to lifetime issues. The read lock of program cache gets dropped when the function returns, and causes the lifetime of inner reference to go out of scope.
The structure carries two Arc<>. So clone of struct is not expensive.

Ahh, okay this was what I was missing. I still prefer the clone at the SVM-level, but I'll leave up to you two.

Looks like all users of LoadedProgram's get_environments_for_epoch() are cloning the reference anyway. So, it should not cause any additional overheads.

[Lichtso]

This introduces a nested read lock (because program_cache is already locked above) which is can cause a dead lock.

[pgarg66]

Yes, created this PR to address it: #1616