feat: faster append_formatted_to_byte_array

[Th0rgal]

This pull request makes bytes_array formatting slightly faster (~10% cheaper in gas usage).
The previous implementation used to append digits to an array via a first loop and loop again over it to reverse it. This code avoids the second iteration by using recursion to reorder the append calls and directly add the digits reversed.
I think it also makes the code cleaner.

cargo run --bin cairo-test -- --single-file ./corelib/src/test/fmt_test.cairo

Before changes:

running 3 tests
test fmt_test::fmt_test::test_array_debug ... ok (gas usage est.: 181390)
test fmt_test::fmt_test::test_format ... ok (gas usage est.: 1201900)
test fmt_test::fmt_test::test_format_debug ... ok (gas usage est.: 1644520)
test result: ok. 3 passed; 0 failed; 0 ignored; 0 filtered out;

After changes:

running 3 tests
test fmt_test::fmt_test::test_array_debug ... ok (gas usage est.: 166690)
test fmt_test::fmt_test::test_format ... ok (gas usage est.: 1071540)
test fmt_test::fmt_test::test_format_debug ... ok (gas usage est.: 1547670)
test result: ok. 3 passed; 0 failed; 0 ignored; 0 filtered out;

---

This change is 

[orizi]

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

---

corelib/src/to_byte_array.cairo line 124 at r1 (raw file):

    append_big_digits_util(@new_value, ref byte_array, base_nz);
    byte_array.append_byte(get_big_base_digit_representation(:digit_as_u8));
}

no need for inline.
and since you pre-checked zero value - you can reorder the functions:

Suggestion:

}

#[inline]
fn append_small_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(
    mut value: @T, ref byte_array: ByteArray, base_nz: NonZero<T>,
) {
    let (new_value, digit) = DivRem::div_rem(*value, base_nz);
    let digit_as_u8: u8 = digit.try_into().unwrap();
    if new_value.is_zero() {
        append_big_digits_util(@new_value, ref byte_array, base_nz);
    }
    byte_array.append_byte(digit_as_u8 + '0');
}

#[inline]
fn append_big_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(
    mut value: @T, ref byte_array: ByteArray, base_nz: NonZero<T>,
) {
    let (new_value, digit) = DivRem::div_rem(*value, base_nz);
    let digit_as_u8: u8 = digit.try_into().unwrap();
    if new_value.is_zero() {
        append_big_digits_util(@new_value, ref byte_array, base_nz);
    }
    byte_array.append_byte(get_big_base_digit_representation(:digit_as_u8));
}

[Th0rgal]

Thanks for the suggestion, let's save one more check. I think you forgot to negate the is_zero check (instead of checking if you want to stop, you check if you want to continue). Here is what I tried (on both big and small digits):

    let (new_value, digit) = DivRem::div_rem(*value, base_nz);
    let digit_as_u8: u8 = digit.try_into().unwrap();
    if !new_value.is_zero() {
        append_big_digits_util(@new_value, ref byte_array, base_nz);
    };
    byte_array.append_byte(get_big_base_digit_representation(:digit_as_u8));

I now get:

running 3 tests
test fmt_test::fmt_test::test_array_debug ... ok (gas usage est.: 166090)
test fmt_test::fmt_test::test_format ... ok (gas usage est.: 1054380)
test fmt_test::fmt_test::test_format_debug ... ok (gas usage est.: 1526890)
test result: ok. 3 passed; 0 failed; 0 ignored; 0 filtered out;

I tried removing the inline but it consumes a bit more gas, not sure why as I would expect it to be inlined automatically:

running 3 tests
test fmt_test::fmt_test::test_array_debug ... ok (gas usage est.: 169690)
test fmt_test::fmt_test::test_format ... ok (gas usage est.: 1059180)
test fmt_test::fmt_test::test_format_debug ... ok (gas usage est.: 1543690)
test result: ok. 3 passed; 0 failed; 0 ignored; 0 filtered out;

Should I remove it anyway?

[orizi]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Th0rgal)

---

corelib/src/to_byte_array.cairo line 100 at r2 (raw file):

}

#[inline]

still remove the inlines - as this is "more efficient" just because it performs one inline of the recursive function - rather not create more bytecode for now.

---

corelib/src/to_byte_array.cairo line 101 at r2 (raw file):

#[inline]
fn append_small_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(

doc

---

corelib/src/to_byte_array.cairo line 113 at r2 (raw file):

#[inline]
fn append_big_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(

doc

[orizi]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Th0rgal)

[Th0rgal]

Interesting, it seems that test_validate_gas_cost because entry_point_gas_usage actually increased, so it might actually not be cheaper for some use cases. Not sure why though.

[orizi]

what is the change exactly?

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

[Th0rgal]

what is the change exactly?

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

before: 141_400
after: 206_880

[orizi]

all the values of the test - not just a singular value.
also - please compare runs with --print_resource_usage to get exact resource usage.

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

[orizi]

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

[Th0rgal]

all the values of the test - not just a singular value. also - please compare runs with --print_resource_usage to get exact resource usage.

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

Thank you, here is what I get from the test, I'll retry with --print_resource_usage :

cargo run --profile=ci-dev --bin cairo-test -- \
    crates/cairo-lang-starknet/cairo_level_tests/ --starknet -f test_validate_gas_cost
    Finished ci-dev [unoptimized + debuginfo] target(s) in 0.43s
     Running `target/ci-dev/cairo-test crates/cairo-lang-starknet/cairo_level_tests/ --starknet -f test_validate_gas_cost`
running 1 test
test cairo_level_tests::abi_dispatchers_tests::test_validate_gas_cost ... fail (gas usage est.: 337080)
failures:
   cairo_level_tests::abi_dispatchers_tests::test_validate_gas_cost - Panicked with "Unexpected gas_usage:
     call_building: `3150`.
     serialization: `50950`.
     entry_point: `154000`.".

Error: test result: FAILED. 0 passed; 1 failed; 0 ignored

[yuvalsw]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Th0rgal)

---

corelib/src/to_byte_array.cairo line 100 at r4 (raw file):

}

/// Appends ascii representation of value in base_nz to byte_array for single digit base.

this is confusing as here you refer to decimal while we may have different bases. Moreover, 10 is not single digit even in decimal base.

Suggestion:

 to byte_array for base <= 10.

---

corelib/src/to_byte_array.cairo line 101 at r4 (raw file):

/// Appends ascii representation of value in base_nz to byte_array for single digit base.
fn append_small_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(

Suggestion:

 +Zeroable<T>>

---

corelib/src/to_byte_array.cairo line 101 at r4 (raw file):

/// Appends ascii representation of value in base_nz to byte_array for single digit base.
fn append_small_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(

Suggestion:

fn append_formatted_to_byte_array_small_base<

---

corelib/src/to_byte_array.cairo line 112 at r4 (raw file):

}

/// Appends ascii representation of value in base_nz to byte_array for any base.

append_formatted_to_byte_array_small_base if the base is <=10 as it's more efficient.

Suggestion:

/// Appends ascii representation of value in base_nz to byte_array for any base.
/// Prefer to use 

---

corelib/src/to_byte_array.cairo line 113 at r4 (raw file):

/// Appends ascii representation of value in base_nz to byte_array for any base.
fn append_big_digits_util<T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(

Suggestion:

fn append_formatted_to_byte_array_big_base

[orizi]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Th0rgal)

---

corelib/src/to_byte_array.cairo line 97 at r4 (raw file):

        assert(base <= 36, 'base must be <= 36');
        append_big_digits_util(value, ref byte_array, base_nz);
    }

Suggestion:

    if base <= LARGE_BASE_REPR_BOUND {
        append_digits_util::<SmallBaseDigitRepr>(value, ref byte_array, base_nz);
    } else {
        assert(base <= 36, 'base must be <= 36');
        append_digits_util::<LargeBaseDigitRepr>(value, ref byte_array, base_nz);
    }

---

corelib/src/to_byte_array.cairo line 110 at r4 (raw file):

    };
    byte_array.append_byte(digit_as_u8 + '0');
}

.

Suggestion:

/// Appends ascii representation of value in base_nz to byte_array for single digit base.
fn append_digits_util<impl repr: DigitRepr, T, +Drop<T>, +Copy<T>, +DivRem<T>, +TryInto<T, u8>, +Zeroable<T>,>(
    mut value: @T, ref byte_array: ByteArray, base_nz: NonZero<T>,
) {
    let (new_value, digit) = DivRem::div_rem(*value, base_nz);
    let digit_as_u8: u8 = digit.try_into().unwrap();
    if !new_value.is_zero() {
        append_digits_util::<repr>(@new_value, ref byte_array, base_nz);
    };
    byte_array.append_byte(repr::repr(digit_as_u8));
}

---

corelib/src/to_byte_array.cairo line 132 at r4 (raw file):

        digit_as_u8 - 10 + 'a'
    }
}

.

Suggestion:

trait DigitRepr {
    fn repr(value: u8) -> u8;
}
impl SmallBaseDigitRepr of DigitRepr {
    fn repr(value: u8) -> u8 {
        '0' + value
    }
}
const LARGE_BASE_REPR_BOUND: u8 = 10;
const LARGE_BASE_REPR_OFFSET: u8 = 'a' - 10;
impl LargeBaseDigitRepr of DigitRepr {
    fn repr(value: u8) -> u8 {
        if value < LARGE_BASE_REPR_BOUND {
            SmallBaseDigitRepr::repr(value)
        } else {
            LARGE_BASE_REPR_OFFSET + value
        }
    }
}