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

Refactor Pulley's interpreter loop #9629

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexcrichton
Copy link
Member

  • Define loop-over-match and loop-with-tail-calls in separate files to make it more clear which is in which (and less #[cfg])
  • Move per-opcode handlers to interp.rs outside of a macro invocation to get better native editor support (e.g. formatting, hints, etc).

This is roughly intended to be perf-neutral but we don't have many automated benchmarks yet for Pulley so it's intended to profile later as well.

* Define loop-over-match and loop-with-tail-calls in separate files to
  make it more clear which is in which (and less `#[cfg]`)
* Move per-opcode handlers to `interp.rs` outside of a macro invocation
  to get better native editor support (e.g. formatting, hints, etc).

This is roughly intended to be perf-neutral but we don't have many
automated benchmarks yet for Pulley so it's intended to profile later as
well.
@alexcrichton alexcrichton requested a review from a team as a code owner November 20, 2024 15:55
@alexcrichton alexcrichton requested review from abrown and removed request for a team November 20, 2024 15:55
@alexcrichton
Copy link
Member Author

cc @fitzgen @Kmeakin

@abrown abrown requested a review from fitzgen November 20, 2024 16:54
@abrown
Copy link
Contributor

abrown commented Nov 20, 2024

@fitzgen, I'll leave this for you for when you get back as I assume you have stronger opinions on this level of changes.

@github-actions github-actions bot added the pulley Issues related to the Pulley interpreter label Nov 20, 2024
Copy link

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "pulley"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: pulley

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@Kmeakin
Copy link
Contributor

Kmeakin commented Nov 20, 2024

cc @fitzgen @Kmeakin

No objections from me

@alexcrichton
Copy link
Member Author

I've done a bit more refactoring here with the goal of modeling the interpreter as an implementation of OpVisitor and ExtendedOpVisitor. This should help improve error messages and type-checking for missing operations when new ones are added.

@alexcrichton
Copy link
Member Author

I think that @fitzgen may be out for another week or so and I'm starting to accumulate other changes built on this, so @abrown would you be ok approving this from a "lgtm" point of view? Pulley is still under heavy development so it should be ok to break things/regress things temporarily and I'm happy to take responsibility for following-up on this with Nick when he's back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pulley Issues related to the Pulley interpreter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants