-
Notifications
You must be signed in to change notification settings - Fork 160
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 logging to the assembler #1139
Conversation
92c2f9b
to
9559364
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few comments inline. Also, I think we should log parsing as well (not just compilation). E.g., "Parsed xyz module in 2.34 ms".
Lastly, could you upload a screenshot of how the output looks like (just one or two examples).
I am not sure that logging of parsing completion will make sense for us, because at the parsing stage we don't have information about name or path of the module, we can obtain this info only at compiling stage. So I can log only something like "Parsed module in 2.34 ms", without specifying the name or the path of the module. |
9559364
to
c27c5fc
Compare
Executing this program
we will get this output without logging level specification:
and this output by specifying level to be
Used |
171259a
to
b9906a3
Compare
One thing I don't like in my implementation is that I have to put the |
b9906a3
to
33e7e20
Compare
33e7e20
to
035511b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good! I left some comments inline.
One thing I don't like in my implementation is that I have to put the
done (* ms)
string to the new line after parsing and compiling regardless of whether we print warnings or information about compiled modules between them.
Yes, agreed. I think we should replace done(* ms)
with more meaningful text.
Another option is to use a more sophisticated logging library - but I'm not sure it is worth it.
035511b
to
cd2aa78
Compare
cd2aa78
to
2fbf62d
Compare
c149976
to
8735dde
Compare
Currently this is how log looks like:
There are a lot of additional information while calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool! Thank you! I left a couple of minor comments inline - but there are two more general comments:
First, I think we should separate logging output from "regular" output. Basically, regular output is something we'd like to see even if we turn the logging off completely. For example, for run
command, it could be something like:
===============================================================================
Run program: miden/examples/fib/fib.masm
-------------------------------------------------------------------------------
Executed program with hash 78d31702eb946e1817e3c0881fcb3739562f08fbddf202de2eae241b9968ab3b in 1 ms
Output: [11112721240812633725]
VM cycles: 3045 extended to 4096 steps (25% padding).
├── Stack rows: 3045
├── Range checker rows: 39
└── Chiplets rows: 337
├── Hash chiplet rows: 336
├── Bitwise chiplet rows: 0
├── Memory chiplet rows: 0
└── Kernel ROM rows: 0
So, I would print the above using regular println
and would use logging for everything else. If we do this, we can set logging default back to WARN
. Basically, if people want to see more detail, they can enable lower levels of logging. With logging enabled, it might looks something like this:
===============================================================================
Run program: miden/examples/fib/fib.masm
-------------------------------------------------------------------------------
INFO ┝━ Reading library files [ 102µs | 0.27% / 2.33% ]
INFO │ ┕━ Reading library file [ 90.4µs | 2.05% ] path: miden/examples/logger/lib/logger_lib.masl
INFO ┝━ Reading program file [ 81.3µs | 0.88% / 1.85% ] path: miden/examples/logger/logger.masm
INFO │ ┕━ Parsing program [ 42.7µs | 0.97% ]
WARN │ ┕━ 🚧 [warn]: warning: unused import: "logger_lib::module2"
INFO ┝━ Compiling program [ 3.35ms | 74.15% / 76.04% ]
INFO │ ┕━ Compile AST [ 83.4µs | 1.14% / 1.89% ]
TRACE │ ┕━ Compiling module [ 33.0µs | 0.75% ] module: "logger_lib::module1"
INFO ┝━ Reading input file [ 67.0µs | 1.52% ]
INFO ┕━ Executing program [ 635µs | 14.41% ]
Executed program with hash 78d31702eb946e1817e3c0881fcb3739562f08fbddf202de2eae241b9968ab3b in 1 ms
Output: [11112721240812633725]
VM cycles: 3045 extended to 4096 steps (25% padding).
├── Stack rows: 3045
├── Range checker rows: 39
└── Chiplets rows: 337
├── Hash chiplet rows: 336
├── Bitwise chiplet rows: 0
├── Memory chiplet rows: 0
└── Kernel ROM rows: 0
We should do the same for other commands as well (obviously, what we consider "regular" will differ from command to command).
Second, for log output formatting, could we do the following:
- Not show the left column with
INFO
,WARN
etc. labels? - For winterfell output, note show the very verbose file location etc. to the right of the message?
So, for the
|
d034374
to
6f73999
Compare
After the update output looks like this:
Output of the However, there are still several problems:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you! I left some comments inline - but they are pretty small.
There is no way we can disable printing the logging level at the beginning of the line (
INFO
,WARN
etc.) without rewriting the whole tree-like output: these levels are placed in the core of this tree. Potentially I can rewrite the whole tree, but I'm not sure that it is worth it.
Yeah - what we have is fine, no need to spend more time on this.
I shortened the unnecessarily detailed lines in the
DEBUG
output of the prover (the ones that were in the VM repo), but most of them are printed from the winterfell implementation of the prover. So to make them all short I need to implement Tracing in the winterfell too, but I'm not sure we want to do that now. Anyway, I can create an issue for that.
I'd like to see how the output of prove
looks like but we probably don't need to do anything else here, and once Winterfell implements tracing, it will all look good again.
6f73999
to
1d4cabd
Compare
72c366c
to
9c30144
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you! Just a couple of minor comments and we can merge.
docs/src/intro/usage.md
Outdated
@@ -62,7 +62,7 @@ Similar to Metal acceleration, SVE/AVX2 acceleration is currently applicable onl | |||
### Running Miden VM | |||
Once the executable has been compiled, you can run Miden VM like so: | |||
``` | |||
./target/optimized/miden [subcommand] [parameters] | |||
[level] ./target/optimized/miden [subcommand] [parameters] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would roll back this change.
docs/src/intro/usage.md
Outdated
@@ -83,6 +83,12 @@ For example: | |||
./target/optimized/miden prove --help | |||
``` | |||
|
|||
It is possible to specify the logging level at the beginning of the script. To set the level the value of the `MIDEN_LOG` environment variable should be specified. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put a a sub-header above this and change the text as follows:
#### Enabling logging
You can use `MIDEN_LOG` environment variable to control how much logging output the VM produces. For example:
9c30144
to
124e026
Compare
This PR implements Tracing logger for the Miden VM.
TODO: