refactor(store): optimize PackedCounter #1231
Conversation
🦋 Changeset detectedLatest commit: 366007d The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| ).toThrowErrorMatchingInlineSnapshot( | ||
| '"PackedCounter \\"0x0000000000004000000000200000000020000000004000000000000000000000\\" total bytes length (64) did not match the summed length of all field byte lengths (128)."' | ||
| '"PackedCounter \\"0x0000000000000000000000000000400000000020000000002000000000000040\\" total bytes length (64) did not match the summed length of all field byte lengths (128)."' |
There was a problem hiding this comment.
ints are right-aligned, it's a bit more optimal to have them packed right to left
There was a problem hiding this comment.
to clarify, you just changed the position order of (uint56, uint40[5]) to (uint40[5], uint56)?
There was a problem hiding this comment.
yeah, but it's important to clarify that uint40[5] is in reverse as well, the first element is the right-most one
dk1a
force-pushed
the
dk1a/packedcounter
branch
from
e609061 to
a64cb12
Compare
|
Pretty sure we'll have more breaking changes (store events, methods, maybe schema), probably best not to add support for old versions each time |
There was a problem hiding this comment.
Very nice savings and code looks good overall. Just had some minor nits and questions. Let's also make sure to add some context on the change from left aligned to right aligned to the changeset (i only understood the reason after looking at the diff of PackedCounter.sol, but we can't expect users to look at the PR diffs so we should explain it in the changeelog)
| @@ -75,25 +75,25 @@ | |||
| "file": "test/Gas.t.sol", | |||
| "test": "testCompareAbiEncodeVsCustom", | |||
| "name": "custom encode", | |||
| "gasUsed": 2806 | |||
| "gasUsed": 1394 | |||
| "file": "test/PackedCounter.t.sol", | ||
| "test": "testAtIndex", | ||
| "name": "get value at index of PackedCounter", | ||
| "gasUsed": 255 | ||
| "gasUsed": 24 | ||
| }, | ||
| { | ||
| "file": "test/PackedCounter.t.sol", | ||
| "test": "testSetAtIndex", | ||
| "name": "set value at index of PackedCounter", | ||
| "gasUsed": 793 | ||
| "test": "testGas", | ||
| "name": "pack 1 length into PackedCounter", | ||
| "gasUsed": 35 |
| } | ||
|
|
||
| /** | ||
| * Set a counter at the given index, return the new packed counter | ||
| */ | ||
| function setAtIndex( | ||
| PackedCounter packedCounter, | ||
| uint256 index, | ||
| uint8 index, |
There was a problem hiding this comment.
it was easier/cheaper than a manual check, and aligns well with how StoreCore also uses uint8 for schemaIndex
Co-authored-by: alvarius <alvarius@lattice.xyz>
| @@ -194,9 +194,11 @@ library NumberList { | |||
|
|
|||
There was a problem hiding this comment.
do templates codegen need to be updated?
There was a problem hiding this comment.
double-checked, nope (I think CI'd catch it). This affects only tables with dynamic fields, which templates don't have
Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>
Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>
Many small tweaks in
PackedCounter.sol,and a big change to pack lengths from right to left (and corresponding changes to sync)