feat: from_array for List

[akhercha]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: #158

What is the new behavior?

• Added the from_array method for ListTrait
• Added the corresponding unit tests

Does this introduce a breaking change?

• Yes
• No

Other information

The implementation is very similar to the append function except that it sets the length only once.

There's one question still, what should happen when you call the function with an empty list, like

lst.from_array(array![]);

For my current implementation it "cleans" the list (basically set the length to zero) but maybe this is not what we want. Let me know! 😁

[0xLucqs]

pop is less efficient than get ?

[akhercha]

pop is less efficient than get ?

With my limited knowledge I'd say pop is better but when doing some tests I found that get without a span had slightly less gaz cosumption than pop.
The results (the "test_from_array_big" test contains 44 elements):

First tests -> using get,
2nd set of tests -> were using span & pop.

Or maybe I should not trust the gaz estimation? @LucasLvy

[0xLucqs]

interesting then maybe implement, a from_span function, and a from_array where from_array is just:

fn from_array(arr: Array<T>) { from_span(arr.span()) }

(not the exact code but you get the idea)

[akhercha]

Updated @LucasLvy !

[akhercha]

I feel like the function can be optimized further by computing base & offset at the beginning & only re-computing base when we overflow the current segment.

I tried & I have something like that currently (the code is still very ugly):

fn from_span(ref self: List<T>, mut span: Span<T>) {
        self.len = span.len();
        if self.len == 0 {
            Store::write(self.address_domain, self.base, self.len);
            return;
        }
        let (mut base, mut offset) = calculate_base_and_offset_for_index(
            self.base, 0, self.storage_size
        );
        let mut segment = 0;
        loop {
            match span.pop_front() {
                Option::Some(v) => {
                    Store::write_at_offset(self.address_domain, base, offset, *v).unwrap_syscall();
                    // TODO: ugly way to avoid any overflow for now
                    let off: u32 = offset.into();
                    let xx: u32 = off + self.storage_size.into();
                    if xx >= POW2_8 {
                        offset = 0;
                        segment += 1;
                        let addr_elements = array![
                            storage_address_to_felt252(storage_address_from_base(self.base)),
                            segment
                        ];
                        base =
                            storage_base_address_from_felt252(
                                poseidon_hash_span(addr_elements.span())
                            );
                    } else {
                        offset += self.storage_size;
                    }
                },
                Option::None => {
                    break;
                }
            };
        };
        Store::write(self.address_domain, self.base, self.len);
    }

It consumes less gaz than my first implementation.
Up -> old implementation,
Bottom -> new implementation.

Are those gas gains worth in comparison to the loss of clarity in the function? 😅

[0xLucqs]

Are those gas gains worth in comparison to the loss of clarity in the function? 😅

I'd say no as i'm not even sure how this gas improvement would translate with the step/builtins fees (currently used on the network)

[akhercha]

Aaah makes sense. I'll just revert those changes then & we'll see if that's needed in the future. Thanks @LucasLvy !

Got lost in the premature optimizations loop yet again 😎

[0xLucqs]

Ok so after asking the sierra gas metering is higher but it consumes less steps/built ins so let’s keep it with pop it’s more optimized for now