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.
| } | ||
|
|
||
| @FunctionalInterface | ||
| private interface Operands { |
3 participants
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.