fix[codegen]: fix overcopying of bytes in make_setter
#4419
Conversation
in `_complex_make_setter`, when a dynamic type (e.g. Bytes or DynArray) is contained within a tuple, the current code generation copies the entire buffer without checking how large it is. (note this represents a gas issue since more bytes might be copied than is necessary, but not a correctness issue). in comparison, Bytes and DynArray make_setters limit the copy length to the runtime sizes of the Bytes/DynArray. this commit disables the full buffer copy and adds a heuristic to the make_setter implementations for DynArray and Bytes so that in certain cases, they copy the full buffer instead of checking the length.
charles-cooper
force-pushed
the
fix/copy-bytes-perf
branch
from
8c5cd16 to
336bc0a
Compare
charles-cooper
changed the title
make_setter
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4419 +/- ##
===========================================
- Coverage 91.76% 51.11% -40.66%
===========================================
Files 119 119
Lines 16615 16634 +19
Branches 2796 2803 +7
===========================================
- Hits 15247 8502 -6745
- Misses 936 7488 +6552
- Partials 432 644 +212 ☔ View full report in Codecov by Sentry. |
| @@ -276,6 +303,9 @@ def _dynarray_make_setter(dst, src, hi=None): | |||
| n_bytes = add_ofst(_mul(count, element_size), 32) | |||
| max_bytes = 32 + src.typ.count * element_size | |||
|
|
|||
| if _prefer_copy_maxbound_heuristic(dst, src, element_size): | |||
| n_bytes = max_bytes | |||
There was a problem hiding this comment.
codecov reports this isn't covered - can you please fix that?
There was a problem hiding this comment.
it's covered -- you can verify by running the test suite with the following patch:
diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py
index 64c6a62a8..74cfebc79 100644
--- a/vyper/codegen/core.py
+++ b/vyper/codegen/core.py
@@ -304,6 +304,7 @@ def _dynarray_make_setter(dst, src, hi=None):
max_bytes = 32 + src.typ.count * element_size
if _prefer_copy_maxbound_heuristic(dst, src, element_size):
+ raise Exception("covered")
n_bytes = max_bytes
# batch copy the entire dynarray, including length wordfor me, this quickly raises an exception at "tests/functional/codegen/types/test_dynamic_array.py::test_dynarray_length_no_clobber[\n@external\ndef should_revert():\n c: DynArray[DynArray[uint256, 1], 2] = [[]]\n c[0] = c.pop()\n ]".
|
the heuristic doesn't seem precise (chatted about this offline). besides that the non-venom part looks sound and shouldn't break invariants. it should be equivalent to having the max_len provided at runtime (if are size calculations are correct) anyway the coverage report suggests these new changes aren't covered by tests, let's please verify that |
in
_complex_make_setter, when a dynamic type (e.g. Bytes or DynArray) is contained within a tuple, the current code generation copies the entire buffer without checking how large it is.(note this represents a gas issue since more bytes might be copied than
is necessary, but not a correctness issue).
in comparison, Bytes and DynArray make_setters limit the copy length to the runtime sizes of the Bytes/DynArray.
this commit disables the full buffer copy and adds a heuristic to the make_setter implementations for DynArray and Bytes so that in certain cases, they copy the full buffer instead of checking the length.
fix an assertion failure in the memmerge pass.
What I did
How I did it
How to verify it
check codesize improvements
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture