Improve immutability of the data structure of the wasm module #501
Conversation
| @@ -127,15 +126,15 @@ public Instance initialize(boolean start) { | |||
| throw new InvalidException( | |||
| "type mismatch, invalid offset type in element " + offset.type()); | |||
| } | |||
| List<List<Instruction>> initializers = ae.initializers(); | |||
| // List<List<Instruction>> initializers = ae.initializers(); | |||
| @@ -16,7 +16,7 @@ public long index() { | |||
| return idx; | |||
| } | |||
|
|
|||
| public List<Instruction> offsetInstructions() { | |||
| return offsetInstructions; | |||
| public Instruction[] offsetInstructions() { | |||
There was a problem hiding this comment.
Why switch to an array? It's already returning an immutable list
| @@ -38,7 +38,7 @@ public int tableIndex() { | |||
| /** | |||
| * @return a constant expression defining the offset into the table | |||
| */ | |||
| public List<Instruction> offset() { | |||
| return offset; | |||
| public Instruction[] offset() { | |||
There was a problem hiding this comment.
Same here, this is returning an immutable list
| @@ -40,8 +40,12 @@ public int labelFalse() { | |||
| return labelFalse; | |||
| } | |||
|
|
|||
| public List<Integer> labelTable() { | |||
| return labelTable; | |||
| public int labelTable(int idx) { | |||
There was a problem hiding this comment.
Hiding the list access is fine, but we do need to make the labelTable list immutable. After making it immutable, the hiding will not be needed.
| @@ -32,16 +33,27 @@ public ValueType type() { | |||
| /** | |||
| * @return the list of instruction lists which are used to initialize each element in the range | |||
| */ | |||
| public List<List<Instruction>> initializers() { | |||
| return initializers; | |||
| public Instruction[] initializer(int idx) { | |||
There was a problem hiding this comment.
We should keep this as a list and change the defensive copy above to copy the inner lists (missed that earlier)
| @@ -22,7 +22,7 @@ public OpCode opcode() { | |||
| } | |||
|
|
|||
| public long[] operands() { | |||
| return operands; | |||
| return operands.clone(); | |||
There was a problem hiding this comment.
For the interpreter, this results in an allocation per opcode with operands. It's possible the JIT could remove the allocation, but based on the benchmark result, it appears this doesn't happen.
I'll put up an alternative PR for this part: #517
|
I think most of these changes are separate from #517. |
|
Cool 👍 thanks for reviewing! |
|
What I meant is that we still need this PR, as most of the changes here are different from the ones in #517. |
|
Ouch, ok, I'll keep it on the Todo list. |
Opening in draft as I expect this PR to have impacts on performance.
@electrum would you like to propose improvements on top of this?