fix[codegen]: fix overcopying of bytes in make_setter#4419
fix[codegen]: fix overcopying of bytes in make_setter#4419charles-cooper merged 28 commits intovyperlang:masterfrom
make_setter#4419Conversation
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4419 +/- ##
=======================================
Coverage 92.70% 92.71%
=======================================
Files 123 123
Lines 17526 17546 +20
Branches 2971 2977 +6
=======================================
+ Hits 16248 16268 +20
Misses 879 879
Partials 399 399 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
HodanPlodky
left a comment
HodanPlodky
left a comment
There was a problem hiding this comment.
Venom part seems good
|
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 |
|
TODO: add heuristic when optimization level is set to |
added in 776fb48 |
charles-cooper
added
release - tentative
vyper/codegen/core.py
Outdated
| length_calc_cost += 15 | ||
|
|
||
| src_bound = src.typ.memory_bytes_required | ||
| if src.location in (CALLDATA, MEMORY) and src_bound <= (5 + length_calc_cost) * 32: |
There was a problem hiding this comment.
why don't we divide by 3 to account for the gas cost of copy op per word?
3 participants
What I did
How I did it
How to verify it
check codesize improvements
note this PR originally had changes to the memmerge pass in venom, but they were superseded by the changes in #4422
Commit message
Description for the changelog
Cute Animal Picture