Copy only after concat args traversal
#620
Conversation
Code generated for the `concat` word is changed to ensure both its arguments are traversed *before* copying their contents onto the resulting sequence. This avoids a partially copied to return buffer should the code fail during resolution of the second argument. It will also allow for a cleaner implementation of cost tracking since we can then also avoid copying on a cost overrun. See-also: #616
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
=======================================
Coverage 85.66% 85.66%
=======================================
Files 46 46
Lines 24597 24592 -5
Branches 24597 24592 -5
=======================================
- Hits 21070 21067 -3
Misses 1675 1675
+ Partials 1852 1850 -2 ☔ View full report in Codecov by Sentry. |
clar2wasm/src/words/sequences.rs
Outdated
| generator.set_expr_type(lhs_expr, ret_ty.clone())?; | ||
| generator.set_expr_type(rhs_expr, ret_ty)?; |
There was a problem hiding this comment.
This is something that has always bothered me here, so maybe it's a good time to change this: the type of the lists arguments shouldn't be exactly the same as ret_ty. When we do this, we stupidly set the arguments max len to the same size as ret_ty. So if the traversal of an argument has to allocate memory, it will allocate the size of ret_ty instead of the real max size of the argument.
There was a problem hiding this comment.
Should I just remove those two lines then? It seems to me like these shouldn't even be here given what you're saying.
There was a problem hiding this comment.
Tests pass with the lines removed
There was a problem hiding this comment.
Normally, you were lucky if the tests pass. if I'm not wrong, we should still have an issue if we try to concatenate two lists with types which would be for example (list 1 (optional Notype)) and (list 1 (optional int)).
clar2wasm/src/words/sequences.rs
Outdated
| /* [] */ | ||
| builder.local_get(ret_off); | ||
| /* [ret_off] */ | ||
| generator.traverse_expr(builder, lhs_expr)?; | ||
| builder.local_tee(lhs_len); | ||
| /* [ret_off, lhs_off, lhs_len] */ | ||
| builder.local_get(ret_off); | ||
| /* [ret_off, lhs_off, lhs_len, ret_off] */ | ||
| builder.local_get(lhs_len); | ||
| /* [ret_off, lhs_off, lhs_len, ret_off, lhs_len] */ | ||
| builder.binop(BinaryOp::I32Add); | ||
| /* [ret_off, lhs_off, lhs_len, ret_off + lhs_len] */ | ||
| generator.traverse_expr(builder, rhs_expr)?; | ||
| builder.local_tee(rhs_len); | ||
| /* [ret_off, lhs_off, lhs_len, ret_off + lhs_len, rhs_off, rhs_len] */ | ||
|
|
||
| // Copy arguments to the return buffer. RHS is copied 1st, and LHS 2nd | ||
|
|
||
| /* [ret_off, lhs_off, lhs_len, ret_off + lhs_len, rhs_off, rhs_len] */ | ||
| builder.memory_copy(memory, memory); | ||
| /* [ret_off, lhs_off, lhs_len] */ |
There was a problem hiding this comment.
You won't believe this, but there are so many comments that my brain can't parse the code :D
Might be just me though.
There was a problem hiding this comment.
Okay, I took a sheet of paper and rewrote this in Wat, it makes sense :)
There was a problem hiding this comment.
I actually do believe you, I used them as aid while rewriting. Would you prefer I remove them? Or perhaps I could leave them, but just on top?
There was a problem hiding this comment.
I tend to never put any, and I don't think I am a good example of documentation 😅
But yeah, maybe removing all the intermediary comments, regrouping all the builder.x into a single expression, and explaining on top what you are calculating could be more readable.
Not sure though, we should try.
Code generated for the
concatword is changed to ensure both its arguments are traversed before copying their contents onto the resulting sequence.This avoids a partially copied to return buffer should the code fail during resolution of the second argument. It will also allow for a cleaner implementation of cost tracking since we can then also avoid copying on a cost overrun.
See-also: #616