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

chore: update Miden VM crates to the commit 1b4609e (next, Sep 5, 2024) #319

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

greenhat
Copy link
Contributor

This fixes #317

@greenhat
Copy link
Contributor Author

@bitwalker
packaging::tests::packaging_deserialization test is failing on Package::read_from_bytes call. Do you by any chance have that fixed somewhere?

test packaging::tests::packaging_deserialization ... FAILED

failures:

---- packaging::tests::packaging_deserialization stdout ----
Error:   x Main thread panicked.
  |-> at codegen/masm/src/packaging/tests.rs:31:51
  `-> called `Result::unwrap()` on an `Err` value:   x entrypoint
      MastNodeId(12) is not a procedure

I added the .unwrap() to catch where the test fails.

@bitwalker
Copy link
Contributor

@greenhat Looks like that must have broken as the result of some recent changes, I'll have to look into it

@bitwalker
Copy link
Contributor

@greenhat Have you identified where in the compiler the issue is caused? It still seems to me that we must be doing something dumb somewhere, or the issue wouldn't exist in the first place. Our changes to cache the standard library, for example, should have ensured that we aren't re-hashing its MAST forest over and over again, just cloning the structure, which isn't super cheap, but definitely not enough to cause the regression.

It also looks to me like the fix I made to cache the standard library isn't working any more, not sure why. But that definitely fixed it before, but I can reproduce the slow tests locally now, and I don't know why it's happening again. I'll take a look if you haven't pinned it down just yet, but wanted to see if you've already found the culprit.

@greenhat
Copy link
Contributor Author

@greenhat Have you identified where in the compiler the issue is caused?

No.

It still seems to me that we must be doing something dumb somewhere, or the issue wouldn't exist in the first place. Our changes to cache the standard library, for example, should have ensured that we aren't re-hashing its MAST forest over and over again, just cloning the structure, which isn't super cheap, but definitely not enough to cause the regression.

I agree. The jump seems too big for a cloning but keep in mind that it in proptests, so x256 and in some cases x1024 apply.

It also looks to me like the fix I made to cache the standard library isn't working any more, not sure why. But that definitely fixed it before, but I can reproduce the slow tests locally now, and I don't know why it's happening again. I'll take a look if you haven't pinned it down just yet, but wanted to see if you've already found the culprit.

Sure. I have not started looking into our side of things anyway.

@bitwalker
Copy link
Contributor

I've identified the underlying issue that has caused the explosion in runtime, and it appears to be primarily related to dropping the Library struct. Switching to next appears to fix the issue, but only because next no longer hands out ownership of StdLibrary (and MidenTxKernelLibrary presumably), instead it clones an Arc that is lazily allocated on first access. This means that using those libraries no longer ever drops them after they are done being used. So once we pick up that change, the immediate issue with our test suite goes away.

However, the issue isn't actually resolved by this change - it will also affect large compilation graphs, or tests which use libraries other than miden-stdlib or miden-base, and once we have toolchain installation implemented, we'll no longer benefit from miden-stdlib and miden-base caching the library in memory.

The "real" fix is to address the performance issues with loading/dropping libraries in Miden VM, but we can at least switch to next temporarily for our sanity.

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.

cargo test takes 8x more time on my laptop after the switch to Miden VM v0.10.5 (from v0.10.3)
2 participants