refactor(byte_array): extract ByteArray::append logic

[giladchase]

The new aux function will be called from the upcoming ByteSpan::to_byte_array().

Note: no logic changes, only extract.

[reviewable-StarkWare]

This change is 

[giladchase]

• feat(ops): add get, SliceIndex and impl for ByteSpan #8417 : 2 dependent PRs (#8427 , #8511 )
• feat(byte_array): add ByteSpan::to_byte_array #8416
• refactor(byte_array): extract ByteArray::append logic #8486 👈 (View in Graphite)
• feat(byte_array): add ByteArray::span and Span::len #8329
• refactor(test): remove fixed-size bytearray testutils #8343
• refactor(test): remove compare_spans util #8342
• refactor(test): remove compare_byte_array util and rework tests #8336
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[giladchase]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions

---

corelib/src/byte_array.cairo line 159 at r1 (raw file):

    /// ```
    fn append(ref self: ByteArray, other: @ByteArray) {
        self.append_aligned_byte_span(other.span());

Performance-wise: almost all bytearray tests increased by ~1K gas, a couple by ~2K.
Adding #[inline] to append_aligned_byte_span improves this to ~300 and ~700, respectively.

The remianing overhead is a result of the span constructor. If we change the signature to the following then no gas usage increases are recorded in the tests.

    fn append_aligned_byte_span(
        ref self: ByteArray,
        mut data: Span<bytes31>,
        pending_word: felt252,
        pending_word_len: usize,
    ) 

Code quote:

     self.append_aligned_byte_span(other.span());

---

corelib/src/byte_array.cairo line 345 at r1 (raw file):

    /// Appends a word-aligned `ByteSpan`.
    /// Requires `span.first_char_start_offset == 0`, otherwise the offset is ignored.

Alternatively we can add anAlignedSpan type that doesn't include start-offset (maybe make span an enum with two variants where one has start-offset and the other doesnt?), which might also save some space when we have spans without start-offsets (for example as a result of the span() constructor).

Downside is that it's a leaky abstraction, users shouldn't care about aligned or not aligned, it's adding complexity.

Code quote:

    /// Requires `span.first_char_start_offset == 0`, otherwise the offset is ignored.

[giladchase]

@orizi Note: tests are failing due to lowering changing as a result of this, but i'm holding off on fixing them until we decide if we want this as is, inlined, and/or without the span-constructor.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions

[giladchase]

Stale, tests pass now.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @orizi)

---

corelib/src/byte_array.cairo line 159 at r1 (raw file):

Previously, giladchase wrote…

Performance-wise: almost all bytearray tests increased by ~1K gas, a couple by ~2K.
Adding #[inline] to append_aligned_byte_span improves this to ~300 and ~700, respectively.

The remianing overhead is a result of the span constructor. If we change the signature to the following then no gas usage increases are recorded in the tests.

    fn append_aligned_byte_span(
        ref self: ByteArray,
        mut data: Span<bytes31>,
        pending_word: felt252,
        pending_word_len: usize,
    ) 

Stale, retracting.

---

corelib/src/byte_array.cairo line 345 at r1 (raw file):

Previously, giladchase wrote…

Alternatively we can add anAlignedSpan type that doesn't include start-offset (maybe make span an enum with two variants where one has start-offset and the other doesnt?), which might also save some space when we have spans without start-offsets (for example as a result of the span() constructor).

Downside is that it's a leaky abstraction, users shouldn't care about aligned or not aligned, it's adding complexity.

Stale, retracting.

[giladchase]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @orizi)

---

corelib/src/byte_array.cairo line 345 at r2 (raw file):

    /// Append `data`` span and `pending_word` fields to the byte array.
    #[inline]

With this the gas usage is unchanged, but if this is omitted we get a slight increase in gas usage (probably too big for the compiler to inline by itself).

Code quote:

    #[inline]

---

corelib/src/byte_array.cairo line 346 at r2 (raw file):

    /// Append `data`` span and `pending_word` fields to the byte array.
    #[inline]
    fn append_from_parts(

Naming is inspired from slice::from_raw_parts, though here it's a bit different since data is a Span, rather than Array<bytes31>

Code quote:

    fn append_from_parts(

[giladchase]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @orizi)

---

corelib/src/byte_array.cairo line 96 at r3 (raw file):

#[generate_trait]
pub impl ByteArrayImpl of ByteArrayTrait {
    // TODO(yuval): add a `new` function for initialization.

Maybe sufficient for resolving this TODO, something like:

fn new(data: Array<bytes31>, pending_word: felt252, pending_len: usize) -> ByteArray {
  let mut ba = Default::default();
  ba.append_from_parts(data.span(), pending_word, pending_len);
  ba
}

Code quote:

    // TODO(yuval): add a `new` function for initialization.

[orizi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

---

corelib/src/byte_array.cairo line 345 at r2 (raw file):

Previously, giladchase wrote…

With this the gas usage is unchanged, but if this is omitted we get a slight increase in gas usage (probably too big for the compiler to inline by itself).

leave for now - but would probably remove soon.

---

corelib/src/byte_array.cairo line 346 at r2 (raw file):

Previously, giladchase wrote…

Naming is inspired from slice::from_raw_parts, though here it's a bit different since data is a Span, rather than Array<bytes31>

add function in a private trait - or just out of a trait completely - to prevent inner detail leak.

---

corelib/src/byte_array.cairo line 96 at r3 (raw file):

Previously, giladchase wrote…

Maybe sufficient for resolving this TODO, something like:

fn new(data: Array<bytes31>, pending_word: felt252, pending_len: usize) -> ByteArray {
  let mut ba = Default::default();
  ba.append_from_parts(data.span(), pending_word, pending_len);
  ba
}

todo can be just removed.

we have both "some-string-literal" as initialization, as well as Default::default.

[giladchase]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @orizi)

---

corelib/src/byte_array.cairo line 346 at r2 (raw file):

Previously, orizi wrote…

add function in a private trait - or just out of a trait completely - to prevent inner detail leak.

Already defined in impl InternalImpl of InternalTrait, unless you mean an additional internal trait def?

[orizi]

@orizi reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

---

corelib/src/byte_array.cairo line 346 at r2 (raw file):

Previously, giladchase wrote…

Already defined in impl InternalImpl of InternalTrait, unless you mean an additional internal trait def?

correct - haven't noticed - resolved.

[orizi]

@orizi reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

[orizi]

@orizi reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

[orizi]

@orizi reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

[graphite-app]

Merge activity

• Oct 8, 9:12 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.