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 module cache test #4285

Merged
merged 7 commits into from
Apr 23, 2024
Merged

Add module cache test #4285

merged 7 commits into from
Apr 23, 2024

Conversation

sisuresh
Copy link
Contributor

Description

Resolves #4284

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@sisuresh sisuresh requested a review from dmkozh April 15, 2024 19:17
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

This checks the refined cost model but not the cache functioning as a cache -- for example that making multiple calls against a single contract costs less than it did before (or doesn't charge multiple parses for the same contract, or whatever).

(We do have tests for that in soroban but it looks like this is an attempt to re-verify some of those properties from the C++ side; if so the stuff in this region of the lifecycle tests covers it)

@sisuresh
Copy link
Contributor Author

This checks the refined cost model but not the cache functioning as a cache -- for example that making multiple calls against a single contract costs less than it did before (or doesn't charge multiple parses for the same contract, or whatever).

(We do have tests for that in soroban but it looks like this is an attempt to re-verify some of those properties from the C++ side; if so the stuff in this region of the lifecycle tests covers it)

Good point Graydon. I updated the test to do a very simple check the the required instruction count fell after the module cache was created. The numbers aren't specific though. What do you think?

@graydon
Copy link
Contributor

graydon commented Apr 22, 2024

@sisuresh Sadly I still think that's just checking that the new cost model is tighter than the old cost model (its tightness causes a cost reduction even if there's no caching). The trick with the module cache is it's only intra-transaction -- it only lives as long as one host -- so you need a testcase that makes multiple calls from contract A to contract B in a single host invocation of contract A.

Sorry for pointing you at those lifecycle tests; they're probably not the most appropriate since you don't have control over host lifetime from here, I forgot that core tests all have to be e2e-style.

For e2e-like tests, I added a specific new test wasm to do this: the sum contract takes a reference to the add contract as an input as well as a vector of numbers, and repeatedly calls add to add them together. like sum(add, [1,2,3,4,5]) will call add(add(add(add(1,2),3),4),5). So if you run this call once under no-module-cache and once under module-cache you will get a much faster overall execution on module-cache. It's used in a recording-fidelity test here though that one in particular is not testing the existence of the module cache, it's just illustrative of the ledger setup you need to have installed -- both the SUM and ADD contracts. The ADD one is the one that gets its cached-ness tested by the multiple calls coming from SUM.

To test for the existence of the module cache, I think you want to upload the ADD and SUM contracts in v20, run them once there to get a baseline cost, then upgrade the host to v21 (to enable module caching at runtime) but not re-upload the contracts (so you're only observing costs related to module caching, not observing a change in costs related to the refined cost model). Then you should (a) see a decline in the costs and (b) see a zero VmCachedInstantiation count in the first invocation and nonzero in the second.

@sisuresh
Copy link
Contributor Author

@sisuresh Sadly I still think that's just checking that the new cost model is tighter than the old cost model (its tightness causes a cost reduction even if there's no caching). The trick with the module cache is it's only intra-transaction -- it only lives as long as one host -- so you need a testcase that makes multiple calls from contract A to contract B in a single host invocation of contract A.

Sorry for pointing you at those lifecycle tests; they're probably not the most appropriate since you don't have control over host lifetime from here, I forgot that core tests all have to be e2e-style.

For e2e-like tests, I added a specific new test wasm to do this: the sum contract takes a reference to the add contract as an input as well as a vector of numbers, and repeatedly calls add to add them together. like sum(add, [1,2,3,4,5]) will call add(add(add(add(1,2),3),4),5). So if you run this call once under no-module-cache and once under module-cache you will get a much faster overall execution on module-cache. It's used in a recording-fidelity test here though that one in particular is not testing the existence of the module cache, it's just illustrative of the ledger setup you need to have installed -- both the SUM and ADD contracts. The ADD one is the one that gets its cached-ness tested by the multiple calls coming from SUM.

To test for the existence of the module cache, I think you want to upload the ADD and SUM contracts in v20, run them once there to get a baseline cost, then upgrade the host to v21 (to enable module caching at runtime) but not re-upload the contracts (so you're only observing costs related to module caching, not observing a change in costs related to the refined cost model). Then you should (a) see a decline in the costs and (b) see a zero VmCachedInstantiation count in the first invocation and nonzero in the second.

Ah yeah I mixed up the module cache and vm instantiation tightening conceptually. I'll add a test for the module cache.

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@graydon
Copy link
Contributor

graydon commented Apr 23, 2024

r+ 2b81648

@latobarita latobarita merged commit 195cf4c into stellar:master Apr 23, 2024
15 checks passed
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.

Test creation of module cache on an existing contract
4 participants