Align the start pointer in ink allocator #2100
Conversation
|
|
||
| /// Aligns the start pointer of the next allocation. | ||
| fn align_ptr(&self, layout: &Layout) -> usize { | ||
| (self.next + layout.align() - 1) & !(layout.align() - 1) |
There was a problem hiding this comment.
Please add a comment with the reasoning behind the calculation and the AND here.
There was a problem hiding this comment.
maybe some concrete example will help here
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑These are the results when building the
Link to the run | Last update: Fri Feb 23 23:35:18 CET 2024 |
crates/allocator/src/bump.rs
Outdated
| /// - Initially `self.next` is `0`` and aligned | ||
| /// - `layout.align() - 1` accounts for `0` as the first index. | ||
| /// - the binary with the inverse of the align ensures | ||
| /// that the next allocated pointer address is of the power of 2. |
There was a problem hiding this comment.
The inverse of the expression align - 1 creates a bitmask that is used to zero out bits, ensuring alignment according to type requirements.
There was a problem hiding this comment.
- The expression
self.next + layout.align() - 1ensures that the pointer is at least as large as the next alignment boundary. - The bitwise AND with
!(layout.align() - 1)clears the lower bits of the result to ensure that the address is a multiple of the alignment. The!operator creates a mask that zeroes out the lower bits that should not contribute to the final aligned address.
|
|
||
| let aligned_size = layout.pad_to_align().size(); |
There was a problem hiding this comment.
Just out of curiosity, how is padding performed for fields in a struct, considering they field types have their own alignment requirements?
There was a problem hiding this comment.
That is not really the concern of the allocator. The allocator will just create an allocation that adheres to the Layout passed. For a struct the alignment will be set to the alignment for the whole struct (AFAIK the alignment of the largest type in the struct). The padding within the struct is given implicitly by the size of the Layout. Rust will just assume the padding when accessing the struct.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2100 +/- ##
==========================================
- Coverage 53.69% 53.63% -0.06%
==========================================
Files 224 224
Lines 7046 7048 +2
Branches 3118 3118
==========================================
- Hits 3783 3780 -3
- Misses 3263 3268 +5 ☔ View full report in Codecov by Sentry. |
|
|
||
| let aligned_size = layout.pad_to_align().size(); |
There was a problem hiding this comment.
That is not really the concern of the allocator. The allocator will just create an allocation that adheres to the Layout passed. For a struct the alignment will be set to the alignment for the whole struct (AFAIK the alignment of the largest type in the struct). The padding within the struct is given implicitly by the size of the Layout. Rust will just assume the padding when accessing the struct.
crates/allocator/src/bump.rs
Outdated
| /// - Initially `self.next` is `0`` and aligned | ||
| /// - `layout.align() - 1` accounts for `0` as the first index. | ||
| /// - the binary with the inverse of the align ensures | ||
| /// that the next allocated pointer address is of the power of 2. |
There was a problem hiding this comment.
- The expression
self.next + layout.align() - 1ensures that the pointer is at least as large as the next alignment boundary. - The bitwise AND with
!(layout.align() - 1)clears the lower bits of the result to ensure that the address is a multiple of the alignment. The!operator creates a mask that zeroes out the lower bits that should not contribute to the final aligned address.
|
|
||
| /// Aligns the start pointer of the next allocation. | ||
| fn align_ptr(&self, layout: &Layout) -> usize { | ||
| (self.next + layout.align() - 1) & !(layout.align() - 1) |
There was a problem hiding this comment.
maybe some concrete example will help here
| let debug_msg = get_res.debug_message(); | ||
| let msgs: Vec<&str> = debug_msg.split('\n').collect(); | ||
| let ptr1 = u64::from_str_radix(msgs[0].trim_start_matches("0x"), 16).unwrap(); | ||
| let ptr2 = u64::from_str_radix(msgs[1].trim_start_matches("0x"), 16).unwrap(); | ||
| let align = u64::from_str_radix(msgs[2], 10).unwrap(); | ||
|
|
There was a problem hiding this comment.
This seems a bit hacky to rely on the debug_message here.
Can't we just do that in the ink! message itself and assert it does not panic, or return the values and assert here?
|
stumbled upon this maybe we can remove the reference to wee_alloc in this PR since you are touching this file |
Summary
Closes #1535
cargo-contractorpallet-contracts?Fixes alignment rules on the ink bump allocator.
Description
Before this PR, the allocator allocated types of different order in sequential order after each other without respecting the alignment and padding rules. While this saved up tiny bits of memory usage, it misaligned the types in memory which can potentially increase direct access.
This PR ensure the valid alignment of datatypes, by correctly aligning the end pointer of the linear memory buffer.
Checklist before requesting a review
CHANGELOG.md