feat: make the Assembler work in debug mode, remove optionality

[huitseeker]

On top of #2366.

This removes the optionality of "turning on" debug mode in the Assembler, fixing #1821. As an unavoidable consequence, we lose the ability to deduplicate between nodes, which are now all equipped with unique AsmOps. — though @plafer may have other ideas on this topic, and this PR could serve as a conversation starter.

We will recover compactness in MastForest representation when we purge such MastForests of decorators and solve #2368.

Adds a test to check the CLI still controls whether the processor (unchanged) still executes in debug mode depending on the --release parameter (this feature is orthogonal to the scope of this PR, we're just documenting its working with this test).

Fixes #1821

[bobbinth]

Sorry - I merged #2366 before looking at this one. Could you merge the latest next into this PR?

[huitseeker]

@bobbinth No worries, I never intend to merge feature branches into each other anyway, all my PRs are always meant to be seen as a queue of things that all merge to main (or in this case next) in a FIFO manner. The rebase needed here is a consequence of the squash merge policy, and that's fine.

[bobbinth]

Looks good! Thank you! I didn't really review the .snap file changes, but covered pretty much everything else and left a few small comments inline.

[plafer]

As an unavoidable consequence, we lose the ability to deduplicate between nodes, which are now all equipped with unique AsmOps.

I'm not sure we fully realized this when we opened the issue. So with #2368 what we're saying is:

• By default, we essentially make all nodes unique so that we can attach some debug information to each.
• If you want release mode for production, you need to compactify the resulting MastForest remove all duplicates.

Is this really what we want? Looking back at the initial conversation, I think what really we wanted was for debug mode to be the default so that we don't need any build.rs shenanigans; but not so much lose the ability to compile directly to release mode. In both cases, we still end up with 2 "types" of MastForest:

• debug mode: larger, set up in a way that you can attach debug infos to each node
• release mode: all duplicate nodes merged, removing the ability to attach debug infos

With this new approach, I just feel like we've made it harder for ourselves to obtain the "release mode MastForest".

[bitwalker]

Just to add some follow up after the sync call today: I think we may be able to get away from needing decorators entirely now that we have operation indexing/ranging. Doing so would probably mean removing the existing debug.* "instructions" entirely, or lower them some other way (especially since there isn't really any advantage to using decorators for them, we could just as easily implement them using events). The trace instruction is all but dead anyway, and the last remaining uses of it by the compiler can be transitioned to other approaches.

So, TL;DR, without decorators, we could move to a model without the need for node fingerprinting (or at least, fingerprinting would only need to deal with side-effecting operations like those that manipulate the advice provider), and all debugging-related information could be stored in a separate data structure from the MAST itself (only needing to provide some means to map operation indices/ranges to associated debug info).

That said, we could make those changes after this is merged, so I don't see it as blocking or anything, just a logical next step.

[bobbinth]

I'll hold off on merging this now (though, I think there are still arguments for doing so) - let's discuss this in more detail during our debug sync (hopefully tomorrow).

[huitseeker]

As requested, here's the script to measure the MastForest size. (use as an example in miden-vm)
Before (6049c56):

=== Miden Core Library Serialized MastForest Size ===
  Total serialized library size: 260,209 bytes
  Total serialized library size: 254.11 KB
  Total serialized library size: 0.25 MB
  Number of MAST nodes: 649
  Number of procedures: 272
  Bytes per MAST node: 400.94
  Bytes per procedure: 956.65

After:

=== Miden Core Library Serialized MastForest Size ===
Total serialized library size: 826039 bytes
Total serialized library size: 806.68 KB
Total serialized library size: 0.79 MB
Number of MAST nodes: 682
Number of procedures: 273
Bytes per MAST node: 1211.20
Bytes per procedure: 3025.78

[plafer]

LGTM!

From the serialization size, we see that only 30 MAST nodes were removed by compaction (~4%), i.e. we don't get a lot of benefit from compacting. Makes sense to me to ask the user to call an explicit compact() method if they really want compaction.

[bobbinth]

=== Miden Core Library Serialized MastForest Size ===
  Total serialized library size: 260,209 bytes
  Total serialized library size: 254.11 KB
  Total serialized library size: 0.25 MB
  Number of MAST nodes: 649
  Number of procedures: 272
  Bytes per MAST node: 400.94
  Bytes per procedure: 956.65

After:

=== Miden Core Library Serialized MastForest Size ===
Total serialized library size: 826039 bytes
Total serialized library size: 806.68 KB
Total serialized library size: 0.79 MB
Number of MAST nodes: 682
Number of procedures: 273
Bytes per MAST node: 1211.20
Bytes per procedure: 3025.78

Very interesting! Thank you for running this comparison! So, the serialized size roughly tripled, but given that the overall size is pretty small, I think this is fine.

A couple of things I found surprising:

• There are under 700 nodes in the entire core library. I would have expected there to be thousands of nodes - but maybe I overestimate our code complexity (the number of procedures looks roughly right though).
• It is interesting that one extra procedure appeared after this PR. Is this because there are two modules somewhere with identical procedures, and previously these were getting naturally de-duplicated?

One other thing that would be good to do is to run our deserialize_libcore() benchmark (in miden-vm crate) to see how this has affected deserialization times. Assuming the impact is not too bad, we should go ahead resolve the conflicts and merge.

[huitseeker]

Comments

Is this because there are two modules somewhere with identical procedures, and previously these were getting naturally de-duplicated?

I would assume so, yes.

Deserialize benchmark (on M4)

origin/next (baseline):

• Time: 11.082 ms - 11.105 ms (average ~11.1 ms)

huitseeker/issue-1821 (this branch):

• Time: 13.413 ms - 13.590 ms (average ~13.5 ms)

Performance Impact: 18% performance degradation; ~2.4ms increase in deserialization time.

[bobbinth]

Performance Impact: 18% performance degradation; ~2.4ms increase in deserialization time.

This looks acceptable - so, I'll merge this PR.

Maybe at some point later on we could experiment with "smart deserialization" - e.g., indicate during deserialization whether we want the debug info or now, and if not, then we'd skip deserializing this part of the MastForest which maybe brings us roughly back to the original numbers.