Skip to content

Generic BitMachine execution tracker #288

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

Merged
merged 2 commits into from
Apr 17, 2025

Conversation

m-kus
Copy link
Contributor

@m-kus m-kus commented Apr 9, 2025

This PR generalizes CaseTracker to ExecTracker that now supports tracking jet calls (in addition to case branches). It is also now possible to run BitMachine with a custom tracker created outside of the crate.

This mechanim allows to implement jet tracer which pretty-prints jet arguments/result for each invocation, see https://github.com/m-kus/simfony/pull/2/files#diff-13e8b158f84f88059e0bd8631dfa9113112e30d7b8c21a15413d0ef36d2ea180

As a follow up it is proposed to further extend ExecTracker trait to allow visitor pattern on case/assert expressions. That would enable debug logging, similar to https://github.com/uncomputable/simfony-webide/blob/adb60d4e60c0d6b019ea13f64b1a166da3467d8d/src/function.rs#L145

Having these two features (jet calls tracking and debug symbols) combined it's fairly simple to output weighted stack traces (e.g. in pprof) and build a Simfony profiler.

Copy link
Collaborator

@uncomputable uncomputable left a comment

Choose a reason for hiding this comment

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

concept ack: 5d77a6e Can the simfony stack trace be implemented with CaseTracker? This should be our goal.

@m-kus
Copy link
Contributor Author

m-kus commented Apr 15, 2025

Can the simfony stack trace be implemented with CaseTracker?

I was thinking of the following scheme:

  • Add new handler for left_assert/case nodes to the Tracker
  • On the Simfony side insert special enter/exit dbg markers into every function
  • Implement custom tracker that maintains call stack

It can be used both for rich error messages and profiling (given jet costs we can aggregate weighted stack traces)

@apoelstra
Copy link
Collaborator

This looks great to me. It's a bit of a sharp API -- your input and output buffers are passed as arrays of UWORDs and to actually read the data you need to extract a bitstream from them (and use the jet's type signature to know where to start) but I think this is fine. It avoids any expensive code needing to be run in the normal no-tracker case. We might want to provide some utility functions to help with this or something.

Will ACK this.

uncomputable
uncomputable previously approved these changes Apr 16, 2025
Copy link
Collaborator

@uncomputable uncomputable left a comment

Choose a reason for hiding this comment

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

ACK 4821b65 I think raw &[UWORD] slices are too low level to provide helpful introspection, but I will leave refactors to future PRs.

@m-kus
Copy link
Contributor Author

m-kus commented Apr 16, 2025

Using &[UWORD] slices is indeed quite low level, but I agree with @apoelstra that it helps to avoid overhead for the no-tracker case. Also it is quite simple to convert to simplicity::BitIter and then leverage existing parsers from the Simfony crate to go from bits to high-level types.

@apoelstra
Copy link
Collaborator

@m-kus if you are motivated, to fix the doc test you need to run with the nightly compiler because stable doesn't have the docsrs flags we need for this test.

If you are less motivated you can drop the CI test and I will do it at some point :)

@m-kus
Copy link
Contributor Author

m-kus commented Apr 17, 2025

Sure, no problem :)
Hope this will fix the pipeline (I tried locally with act but it's not straightforward because I'm on arm)

@apoelstra
Copy link
Collaborator

Great, looks good! I have queued this on my local CI box. There is one rust-bitcoin PR in front of it so it'll hopefully finish in the next hour.

Very cool that you're developing on an arm box! We definitely appreciate the extra testing we get from you doing this..

Copy link
Collaborator

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 36ff961; successfully ran local tests

@apoelstra
Copy link
Collaborator

@uncomputable can I go ahead and merge this?

- name: Checkout Crate
uses: actions/checkout@v4
- name: Checkout Toolchain
uses: dtolnay/rust-toolchain@nightly
Copy link
Collaborator

Choose a reason for hiding this comment

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

36ff961: an unpinned nightly version might lead to flaky CI, but it should be ok for checking if docs build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have one :). I figured we could pin both instances at once. (Which we should do!)

Copy link
Collaborator

@uncomputable uncomputable left a comment

Choose a reason for hiding this comment

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

ACK 36ff961

@uncomputable uncomputable merged commit bef2d03 into BlockstreamResearch:master Apr 17, 2025
23 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.

3 participants