Merge locals and label values in Frame#605
Conversation
test result: FAILED. 71 passed; 17 failed; 2 ignored; 0 measured; 0 filtered out; finished in 3.91s
failures:
block
br
br_if
br_table
call
call_indirect
fac
global
if_
labels
left_to_right
local_tee
loop_
select
switch
unreachable
unwind
failures:
br
br_if
br_table
if_
labels
loop_
switch
unreachable
unwind
test result: FAILED. 79 passed; 9 failed; 2 ignored; 0 measured; 0 filtered out; finished in 5.16s
| } | ||
| unsafe { self.parser.restore(frame.ret) }; | ||
| self.values().extend(frame.labels_values); | ||
| self.values().extend(frame.values); |
There was a problem hiding this comment.
@ia0 You mentioned earlier that this line could be optimized. Here self.values() refers to the values of the frame before the popped frame, and it does not know about frame.values. Could you explain more? Thanks.
There was a problem hiding this comment.
This will get optimized once we use a single value stack for the whole thread (not just per function frame). But this will only be true if we go with the "keep locals outside the stack" option (discussed above). Otherwise we'll have to drain.
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
LGTM, although it's not clear yet whether we want to keep this in the end.
| debug_assert_eq!(frame.labels_values.len(), label.values_cnt); | ||
| debug_assert_eq!(frame.labels_values.len(), frame.arity); | ||
| let mut frame = self.frames.pop().unwrap(); | ||
| frame.values.drain(0 .. frame.locals_cnt); |
There was a problem hiding this comment.
I'm not sure what's the best here. We might want to try both options (and look what others are doing). It looks like we can either:
- keep the locals out of the stack (thus saving this drain)
- keep the locals inside the stack (thus saving popping them when calling a function)
The main difference is that draining is proportional in the number of remaining elements, while popping is proportional to what is popped. But I expect both of those numbers to be rather small, so maybe it doesn't matter. Just something to keep in mind once we have something working and start doing benchmarks.
There was a problem hiding this comment.
Thanks for the explanation. I'll add a TODO for this tradeoff in the next PR.
| } | ||
| unsafe { self.parser.restore(frame.ret) }; | ||
| self.values().extend(frame.labels_values); | ||
| self.values().extend(frame.values); |
There was a problem hiding this comment.
This will get optimized once we use a single value stack for the whole thread (not just per function frame). But this will only be true if we go with the "keep locals outside the stack" option (discussed above). Otherwise we'll have to drain.
Reverted #605 and keep values separate from locals for now. #46 --------- Co-authored-by: Zhou Fang <[email protected]> Co-authored-by: Julien Cretin <[email protected]> Co-authored-by: Julien Cretin <[email protected]>
2 participants
Continuation of #598 towards adding a value stack in
Thread.#46