Make Instruction class immutable #517
Conversation
There was a problem hiding this comment.
I like those changes!
Unfortunately, it looks like they are introducing a noticeable performance regression:
https://github.com/dylibso/chicory/actions/runs/10754183983?pr=517
It would be great if you can verify locally if the data is accurate or we are just looking at different machines noise levels.
electrum
force-pushed
the
instruction
branch
from
d0ffa45
to
b40ba24
Compare
|
I missed a critical place in the interpreter that calls |
electrum
force-pushed
the
instruction
branch
from
b40ba24
to
e5850ea
Compare
electrum
force-pushed
the
instruction
branch
from
e5850ea
to
c12d39f
Compare
|
This should be fixed now. I added a private |
|
|
good catch @andreaTP :) |
| var operands = instruction.operands(); | ||
| instance.onExecution(instruction, operands, stack); | ||
| Operands operands = instruction::operand; | ||
| instance.onExecution(instruction, stack); |
There was a problem hiding this comment.
nice clean up!
operands was redundant here.
| @@ -1982,4 +1982,9 @@ private static void checkInterruption() { | |||
| throw new ChicoryException("Thread interrupted"); | |||
| } | |||
| } | |||
|
|
|||
| @FunctionalInterface | |||
| private interface Operands { | |||
This keeps the
long[]to avoid boxing, which might have a performance impact for the interpreter. It also adds a constant for the common case of an empty array, which avoids allocations and reduces memory usage.The extra clone for instructions created during parsing could be eliminated by adding a trusted package-private constructor, but I hope we don't need to optimize to that level.