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

[WIP] Attempt to revamp the closure interpreter #448

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andreaTP
Copy link
Collaborator

Now that we have a mean to measure with JMH I rebased @enebo 's effort to latest main:
https://github.com/enebo/chicory/tree/closure_intpreter

I haven't noticed a massive performance improvement, thought, will check on the CI results again.

@electrum
Copy link
Collaborator

My initial thought is that this is more difficult to read, debug, etc. Given we have AOT, keeping the interpreter simpler seems better, even if it's slightly slower. It depends on the goals for the project.

@andreaTP
Copy link
Collaborator Author

I don't want to push for this, I agree that this is slightly more difficult to read/debug etc., the CI also doesn't show any game-changing performance improvement ( ref ).

It was worth giving it a try to make sure we have not been missing an opportunity.

@electrum
Copy link
Collaborator

If I'm reading the benchmark correctly, it's actually worse by 23% and 36%?

Doing these experiments is helpful. I think it shows that the JVM is good at optimizing and that simpler code is often easier for the JVM to understand.

From a code perspective, you could structure this similar to the existing switch statement by keeping the static methods:

instructions[OpCode.I32_GE_U.ordinal()] =
        (frame, stack, instance, callStack, instruction, operands) -> I32_GE_U(stack);

With Java 21, this could be simplified to

instructions[OpCode.I32_GE_U.ordinal()] =
        (_, stack, _, _, _, _) -> I32_GE_U(stack);

But I like the switch better (and seems easier for the JVM to understand).

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.

2 participants