feat(corelib): iter::adapters::Map

[julio4]

This is mostly an exploratory PR to test implementation of iterator adapters, i.e. functions that take an Iterator to return another Iterator. It's really useful as any type that implement IntoIterator can benefits from these adapters, and it also enables function chaining on iterators.

Added a first implementation of iter::adapters::Map:

let mut i = 1;
for elem in (1..4_u8).into_iter().map(|x| 2 * x).map(|x| x + 1) {
    assert_eq!(elem, i * 2 + 1);
    i += 1;
}

---

I'm curious to know if an implementation of ops::FnMut would be possible, to be able to perform function calls with a reference? With it I believe the efficiency of these adapters can be greatly improved to avoid excessive copy/drop.

Feedback welcome on both the current implementation and potential optimization

[reviewable-StarkWare]

This change is 

[orizi]

Reviewed 2 of 6 files at r1, all commit messages.
Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @julio4)

---

corelib/src/iter/traits/iterator.cairo line 12 at r1 (raw file):

    #[inline]
    fn map<

doc.

---

corelib/src/iter/adapters/map.cairo line 2 at r1 (raw file):

use crate::iter::Iterator;

doc all around.

---

corelib/src/iter/adapters/map.cairo line 24 at r1 (raw file):

    +core::ops::FnOnce<F, (TIter::Item,)>[Output: B],
    +Drop<I>,
    +Drop<F>,

why Drop and not Destruct all around?

Code quote:

    +Drop<I>,
    +Drop<F>,

---

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

    +Drop<I>,
    +Drop<F>,
    +Copy<F>,

why is Copy required?

Code quote:

    +Copy<F>,

[julio4]

corelib/src/iter/adapters/map.cairo line 2 at r1 (raw file):

Previously, orizi wrote…

doc all around.

Done.

[julio4]

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, orizi wrote…

why is Copy required?

When calling map(self.f), self.f is of type FnOnce so it is moved:

    fn map<U, F, +Destruct<F>, +core::ops::FnOnce<F, (T,)>[Output: U]>(
        self: Option<T>, f: F,
    ) -> Option<U>;

Maybe we could use Fn instead of FnOnce to not have to copy one new instance of f at every iteration

[julio4]

corelib/src/iter/adapters/map.cairo line 24 at r1 (raw file):

Previously, orizi wrote…

why Drop and not Destruct all around?

I am unsure what is the best. But I changed to Destruct

[julio4]

Reviewable status: 1 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi)

---

corelib/src/iter/traits/iterator.cairo line 12 at r1 (raw file):

Previously, orizi wrote…

doc.

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 1 of 7 files reviewed, 2 unresolved discussions (waiting on @julio4)

---

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, julio4 (Julio) wrote…

When calling map(self.f), self.f is of type FnOnce so it is moved:

    fn map<U, F, +Destruct<F>, +core::ops::FnOnce<F, (T,)>[Output: U]>(
        self: Option<T>, f: F,
    ) -> Option<U>;

Maybe we could use Fn instead of FnOnce to not have to copy one new instance of f at every iteration

oh - you should definitely use Fn and not FnOnce in the iterator map case.

---

a discussion (no related file):
@enitrat for doc 2nd eye

[enitrat]

Reviewable status: 1 of 7 files reviewed, 13 unresolved discussions (waiting on @julio4 and @orizi)

---

a discussion (no related file):

Previously, orizi wrote…

@enitrat for doc 2nd eye

"Adapters" are "Functions which take an Iterator and return another Iterator". Thus Map is not an adapter as it returns

---

corelib/src/iter.cairo line 25 at r2 (raw file):

//! [Traits]: #traits
//! [Functions]: #functions
//! [Structs]: #structs

we'll figure this out better once scarb doc updates but I think this might need to refer the full path to the modules

---

corelib/src/iter.cairo line 57 at r2 (raw file):

//! [`next`]: Iterator::next
//!
//! # Iterators in Cairo

Suggestion:

//! # Forms of iteration

---

corelib/src/iter.cairo line 59 at r2 (raw file):

//! # Iterators in Cairo
//!
//! There are currently one common method which can create iterators from a collection:

Suggestion:

//! There is currently only one common method which can create iterators from a collection:

---

corelib/src/iter.cairo line 141 at r2 (raw file):

//!
//! This will print the numbers one through five, each on their own line. But
//! you'll notice something here: we never called anything on our vector to

Suggestion:

//! you'll notice something here: we never called anything on our array to

---

corelib/src/iter.cairo line 144 at r2 (raw file):

//! produce an iterator. What gives?
//!
//! There's a trait in the standard library for converting something into an

Suggestion:

//! There's a trait in the core library for converting something into an

---

corelib/src/iter.cairo line 150 at r2 (raw file):

//! it into:
//!
//! [`into_iter`]: IntoIterator::into_iter

might need the full path here

---

corelib/src/iter.cairo line 179 at r2 (raw file):

//!     result
//! }
//! ```

@orizi is this accurate for Cairo as well?

Code quote:

//! ```
//! let values = array![1, 2, 3, 4, 5];
//! {
//!     let mut iter = IntoIterator::into_iter(values);
//!     let result = loop {
//!             let mut next = 0;
//!             match iter.next() {
//!                 Option::Some(val) => next = val,
//!                 Option::None => {
//!                     break;
//!                 },
//!             };
//!             let x = next;
//!             let () = { println!("{x}"); };
//!         };
//!     result
//! }
//! ```

---

corelib/src/iter.cairo line 185 at r2 (raw file):

//! that point, we `break` out of the loop, and we're done iterating.
//!
//! There's one more subtle bit here: the standard library contains an

Suggestion:

//! There's one more subtle bit here: the core library contains an

---

corelib/src/iter.cairo line 209 at r2 (raw file):

//!
//! If an iterator adapter panics, the iterator will be in an unspecified (but
//! memory safe) state.

Not sure this applies

Code quote:

//! If an iterator adapter panics, the iterator will be in an unspecified (but
//! memory safe) state.

---

corelib/src/option.cairo line 525 at r2 (raw file):

    /// assert!(x.map(|s: ByteArray| s.len()) == Option::None);
    /// ```
    fn map<U, F, +Destruct<F>, +core::ops::FnOnce<F, (T,)>[Output: U]>(

update trait as well

---

corelib/src/option.cairo line 513 at r2 (raw file):

    /////////////////////////////////////////////////////////////////////////
    // Transforming contained values
    /////////////////////////////////////////////////////////////////////////

restore this header

Code quote:

    /////////////////////////////////////////////////////////////////////////
    // Transforming contained values
    /////////////////////////////////////////////////////////////////////////

[enitrat]

Reviewable status: 1 of 7 files reviewed, 13 unresolved discussions (waiting on @julio4 and @orizi)

---

a discussion (no related file):

Previously, enitrat (Mathieu) wrote…

"Adapters" are "Functions which take an Iterator and return another Iterator". Thus Map is not an adapter as it returns

disregard this comment, I didn't finish to type it, it's fine

[julio4]

corelib/src/iter.cairo line 25 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

we'll figure this out better once scarb doc updates but I think this might need to refer the full path to the modules

What format should I use for now?

[julio4]

Reviewable status: 1 of 7 files reviewed, 13 unresolved discussions (waiting on @enitrat and @orizi)

---

corelib/src/iter.cairo line 150 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

might need the full path here

Done.

---

corelib/src/iter.cairo line 179 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

@orizi is this accurate for Cairo as well?

I did a quick translation of the rust documentation and have no idea how it's done in Cairo. We can remove this part maybe.

---

corelib/src/iter.cairo line 209 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

Not sure this applies

Removed as I'm not sure as well

---

corelib/src/option.cairo line 513 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

restore this header

Done.
I don't like the way it's show up in the documentation hover by the language server so I removed it and forgot to revert.
It's more a lsp issue

---

corelib/src/option.cairo line 525 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

update trait as well

Done.

---

corelib/src/iter.cairo line 57 at r2 (raw file):

//! [`next`]: Iterator::next
//!
//! # Iterators in Cairo

Done.

---

corelib/src/iter.cairo line 59 at r2 (raw file):

//! # Iterators in Cairo
//!
//! There are currently one common method which can create iterators from a collection:

Done.

---

corelib/src/iter.cairo line 141 at r2 (raw file):

//!
//! This will print the numbers one through five, each on their own line. But
//! you'll notice something here: we never called anything on our vector to

Done.

---

corelib/src/iter.cairo line 144 at r2 (raw file):

//! produce an iterator. What gives?
//!
//! There's a trait in the standard library for converting something into an

Done.

---

corelib/src/iter.cairo line 185 at r2 (raw file):

//! that point, we `break` out of the loop, and we're done iterating.
//!
//! There's one more subtle bit here: the standard library contains an

Done.

[orizi]

FnMut will be possible in the future, but won't actually affect the efficiency, as Cairo has an immutable memory model, so all mutability is simulates, so even given FnMut, it would not be "reference based" as there are no references.

Reviewed all commit messages.
Reviewable status: 1 of 7 files reviewed, 13 unresolved discussions (waiting on @enitrat and @julio4)

---

corelib/src/iter.cairo line 179 at r2 (raw file):

Previously, julio4 (Julio) wrote…

I did a quick translation of the rust documentation and have no idea how it's done in Cairo. We can remove this part maybe.

It is correct though.
while, while let, and for are the same here as in rust.

[julio4]

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, orizi wrote…

oh - you should definitely use Fn and not FnOnce in the iterator map case.

Yes figured that out and updated the implementation.

One thing I wasn't able to figure out is how to use an Fn as an FnOnce.
Option::map takes a FnOnce:

    fn map<U, F, +Destruct<F>, +core::ops::FnOnce<F, (T,)>[Output: U]>(
        self: Option<T>, f: F,
    ) -> Option<U>;

And there's an implementation of FnOnce for types that implement Fn:

/// An implementation of `FnOnce` when `Fn` is implemented.
/// Makes sure we can always pass an `Fn` to a function that expects an `FnOnce`.
impl FnOnceImpl<T, Args, +Destruct<T>, +Fn<T, Args>> of FnOnce<T, Args> {
    type Output = Fn::<T, Args>::Output;
    fn call(self: T, args: Args) -> Self::Output {
        Fn::call(@self, args)
    }
}

But this don't works:

    fn next(ref self: Map<I, F>) -> Option<func::Output> {
        self.iter.next().map(@self.f)
    }

So I propose adding a new FnOnceSnapImpl:

/// An implementation of `FnOnce` when `Fn` is implemented and the receiver is by-reference.
/// Makes sure we can always pass an `Fn` to a function that expects an `FnOnce`.
impl FnOnceSnapImpl<T, Args, +Destruct<T>, +Fn<T, Args>> of FnOnce<@T, Args> {
    type Output = Fn::<T, Args>::Output;
    fn call(self: @T, args: Args) -> Self::Output {
        Fn::call(self, args)
    }
}

If there's a better way to do it let me know.

[julio4]

And here's some benchmark:
A:

let a = array![1, 2, 3];
let mut iter = a.into_iter().map(|x| 2 * x);
let _ = iter.next();
let _ = iter.next();
let _ = iter.next();

B:

let a = array![1, 2, 3];
let mut b = array![];
for x in a {
    b.append(2 * x);
};

A: gas usage est.: 3000
B: gas usage est.: 9680

So this could be used for optimizing some logic

[julio4]

This makes sense, the optimization was mostly not copying each function with Fn. Thank you for the clear explanation!

[julio4]

corelib/src/iter.cairo line 179 at r2 (raw file):

Previously, orizi wrote…

It is correct though.
while, while let, and for are the same here as in rust.

It is slightly different than the rust one though, where I tried to adapt it to Cairo syntax as closely as possible

[orizi]

Reviewable status: 1 of 8 files reviewed, 12 unresolved discussions (waiting on @enitrat)

---

a discussion (no related file):
Adding @TomerStarkware for 2nd eye, and more insights for the FnOnce vs Fn stuff.

[orizi]

Reviewable status: 1 of 8 files reviewed, 12 unresolved discussions (waiting on @enitrat)

---

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, julio4 (Julio) wrote…

Yes figured that out and updated the implementation.

One thing I wasn't able to figure out is how to use an Fn as an FnOnce.
Option::map takes a FnOnce:

    fn map<U, F, +Destruct<F>, +core::ops::FnOnce<F, (T,)>[Output: U]>(
        self: Option<T>, f: F,
    ) -> Option<U>;

And there's an implementation of FnOnce for types that implement Fn:

/// An implementation of `FnOnce` when `Fn` is implemented.
/// Makes sure we can always pass an `Fn` to a function that expects an `FnOnce`.
impl FnOnceImpl<T, Args, +Destruct<T>, +Fn<T, Args>> of FnOnce<T, Args> {
    type Output = Fn::<T, Args>::Output;
    fn call(self: T, args: Args) -> Self::Output {
        Fn::call(@self, args)
    }
}

But this don't works:

    fn next(ref self: Map<I, F>) -> Option<func::Output> {
        self.iter.next().map(@self.f)
    }

So I propose adding a new FnOnceSnapImpl:

/// An implementation of `FnOnce` when `Fn` is implemented and the receiver is by-reference.
/// Makes sure we can always pass an `Fn` to a function that expects an `FnOnce`.
impl FnOnceSnapImpl<T, Args, +Destruct<T>, +Fn<T, Args>> of FnOnce<@T, Args> {
    type Output = Fn::<T, Args>::Output;
    fn call(self: @T, args: Args) -> Self::Output {
        Fn::call(self, args)
    }
}

If there's a better way to do it let me know.

@TomerStarkware

[TomerStarkware]

Reviewable status: 1 of 8 files reviewed, 12 unresolved discussions (waiting on @enitrat and @orizi)

---

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, orizi wrote…

@TomerStarkware

I agree with adding FnOnceSnapImpl, and it looks like Rust is doing that as well,
it might be better to have it implement Fn<@t,Args> and then the regular FnOnceImpl will work

[enitrat]

Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @julio4, @orizi, and @TomerStarkware)

---

corelib/src/iter.cairo line 25 at r2 (raw file):

Previously, julio4 (Julio) wrote…

What format should I use for now?

let's keep this for now

---

corelib/src/option.cairo line 513 at r2 (raw file):

Previously, julio4 (Julio) wrote…

Done.
I don't like the way it's show up in the documentation hover by the language server so I removed it and forgot to revert.
It's more a lsp issue

can you open a bug report?

---

corelib/src/iter/traits/iterator.cairo line 79 at r2 (raw file):

    ///
    /// ```
    /// # #![allow(unused_must_use)]

does this tag exist?

[julio4]

Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @enitrat, @orizi, and @TomerStarkware)

---

corelib/src/option.cairo line 513 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

can you open a bug report?

Yes I already opened it

---

corelib/src/iter/traits/iterator.cairo line 79 at r2 (raw file):

Previously, enitrat (Mathieu) wrote…

does this tag exist?

No it don't exist

[julio4]

corelib/src/iter/adapters/map.cairo line 25 at r1 (raw file):

Previously, TomerStarkware wrote…

I agree with adding FnOnceSnapImpl, and it looks like Rust is doing that as well,
it might be better to have it implement Fn<@t,Args> and then the regular FnOnceImpl will work

@TomerStarkware I added it in another PR: #6992

[orizi]

Reviewed 3 of 3 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @enitrat, @julio4, and @TomerStarkware)

---

corelib/src/iter/traits/iterator.cairo line 79 at r2 (raw file):

Previously, julio4 (Julio) wrote…

No it don't exist

do a let _ = ... instead - valid code, and explains the same effect.

---

corelib/src/iter/adapters/map.cairo line 23 at r5 (raw file):

        Map { iter, f }
    }
}

consider - just to avoid the confusion with the starknet storage's MapTrait,
or just having a non-member function - as you don't even use self in it.

Suggestion:

#[must_use]
#[derive(Drop, Clone)]
pub struct Map<I, F> {
    pub(crate) iter: I,
    pub(crate) f: F,
}
//// or:
pub(crate) fn mapped_iterator<I, F>(iter: I, f: F) -> Map<I, F> {
    Map { iter, f }
}

[julio4]

Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @enitrat, @orizi, and @TomerStarkware)

---

corelib/src/iter/adapters/map.cairo line 23 at r5 (raw file):

Previously, orizi wrote…

consider - just to avoid the confusion with the starknet storage's MapTrait,
or just having a non-member function - as you don't even use self in it.

Done.

---

corelib/src/iter/traits/iterator.cairo line 79 at r2 (raw file):

Previously, orizi wrote…

do a let _ = ... instead - valid code, and explains the same effect.

Done.

[enitrat]

Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)

[orizi]

Reviewed 1 of 4 files at r6.
Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @enitrat, @julio4, and @TomerStarkware)

---

corelib/src/iter/adapters/map.cairo line 20 at r6 (raw file):

pub(crate) fn mapped_iterator<I, F>(iter: I, f: F) -> Map<I, F> {
    Map { iter, f }
}

only one pub(crate) in the path is relevant.

this is contained in a private module.

Suggestion:

pub fn mapped_iterator<I, F>(iter: I, f: F) -> Map<I, F> {
    Map { iter, f }
}

---

corelib/src/test/iter_test.cairo line 1 at r6 (raw file):

use crate::iter::{IntoIterator, Iterator};

rebase - in prelude now.

---

corelib/src/iter/adapters.cairo line 2 at r6 (raw file):

mod map;
pub use map::{Map, mapped_iterator};

this just made mapped_iterator pub.

Suggestion:

pub use map::Map;
pub(crate) use map::mapped_iterator;

---

corelib/src/iter/traits/iterator.cairo line 82 at r6 (raw file):

    /// let _ = (0..5_usize).into_iter().map(|x| println!("{x}"));
    ///
    /// // it won't even execute, as it is lazy. Cairo will warn you about this.

or something similar (as it doesn't in this case, due to the let _ =

Suggestion:

    /// // it won't even execute, as it is lazy. Cairo will warn you about this if not specifically ignored, as is done here..

[enitrat]

Reviewable status: 4 of 8 files reviewed, 5 unresolved discussions (waiting on @julio4, @orizi, and @TomerStarkware)

---

corelib/src/iter/adapters/map.cairo line 20 at r6 (raw file):

Previously, orizi wrote…

only one pub(crate) in the path is relevant.

this is contained in a private module.

why are we marking this as public when we don't have the intent of making this accessible (as the module is private)?

[julio4]

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @enitrat, @orizi, and @TomerStarkware)

---

corelib/src/iter/adapters.cairo line 2 at r6 (raw file):

Previously, orizi wrote…

this just made mapped_iterator pub.

Done.
I just don't know why I had warning unused import: core::iter::adapters::mapped_iterator but it was used it Iterator

---

corelib/src/iter/adapters/map.cairo line 20 at r6 (raw file):

Previously, enitrat (Mathieu) wrote…

why are we marking this as public when we don't have the intent of making this accessible (as the module is private)?

We make it public in the scope of the map module, so that in adapters mod we can use it and make it public for the crate?

---

corelib/src/iter/traits/iterator.cairo line 82 at r6 (raw file):

Previously, orizi wrote…

or something similar (as it doesn't in this case, due to the let _ =

Done.

---

corelib/src/test/iter_test.cairo line 1 at r6 (raw file):

Previously, orizi wrote…

rebase - in prelude now.

Done.

[orizi]

Reviewed 1 of 2 files at r4, 4 of 4 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @julio4)

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

And here's some benchmark:
A:

let a = array![1, 2, 3];
let mut iter = a.into_iter().map(|x| 2 * x);
let _ = iter.next();
let _ = iter.next();
let _ = iter.next();

B:

let a = array![1, 2, 3];
let mut b = array![];
for x in a {
    b.append(2 * x);
};

A: gas usage est.: 3000
B: gas usage est.: 9680

So this could be used for optimizing some logic

Can you please rebase and check again?
Also, note that loops has their own overhead in general.

---

corelib/src/ops/function.cairo line 27 at r8 (raw file):

/// An implementation of `FnOnce` when `Fn` is implemented and the receiver is by-reference.
/// Makes sure we can always pass an `Fn` to a function that expects an `FnOnce`.

rebase please.

[julio4]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

---

a discussion (no related file):

Previously, orizi wrote…

Can you please rebase and check again?
Also, note that loops has their own overhead in general.

Yes, I considered loops because in most case I think users will use for loops.
Here's maybe a more accurate benchmark:

A

let a: Array<u8> = array![1, 2, 3];
let mut iter = a.into_iter().map(|x| 2 * x);
loop {
    if let Option::Some(x) = iter.next() {
        x;
    } else {
        break;
    }
};

B

let a: Array<u8> = array![1, 2, 3];
let mut iter = a.into_iter();
loop {
    if let Option::Some(x) = iter.next() {
        x * 2;
    } else {
        break;
    }
};

C

let a: Array<u8> = array![1, 2, 3];
for x in a {
    2 * x;
};

A: gas usage est.: 12090
B: gas usage est.: 10290
C: gas usage est.: 9990

There's a overhead with map, and it seems to be linear with the size of the iterator.
Using a RangeIterator of size 30 gives:

A: gas usage est.: 153620
B: gas usage est.: 80870
C: gas usage est.: 83870

However the strengths of iterators is the ability to define mutation in a declarative way, and using composition/chaining with multiple adapters (filter, take, ...), so it's still worthwhile to try to optimize in my opinion.

Let's take a last example where we need to perform two distinct mutation over an array.
With imperative approach, we have to use an intermediate array (supposing that we're not minimizing the two mutations in only one):

A

let mut iter = (0..100_u8)
    .into_iter()
    .map(|x| {
        if (x % 2) == 0 {
            x * 2
        } else {
            x + 1
        }
    })
    .map(|x| x + 1);

let mut collect = array![];
for x in iter {
    collect.append(x);
}

B

let mut iter = (0..100_u8).into_iter();

let mut intermediate_arr = array![];
for x in iter {
    if (x % 2) == 0 {
        intermediate_arr.append(x * 2);
    } else {
        intermediate_arr.append(x + 1);
    }
};

let mut collect = array![];
for x in intermediate_arr {
    collect.append(x + 1);
}

A: gas usage est.: 748030
B: gas usage est.: 747140

Given the map iterator overhead, it is same performance as creating a new intermediate array.
Do you think there could be way to optimize iterator performances?

---

corelib/src/ops/function.cairo line 27 at r8 (raw file):

Previously, orizi wrote…

rebase please.

Done.

[orizi]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @julio4)

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

Yes, I considered loops because in most case I think users will use for loops.
Here's maybe a more accurate benchmark:

A

let a: Array<u8> = array![1, 2, 3];
let mut iter = a.into_iter().map(|x| 2 * x);
loop {
    if let Option::Some(x) = iter.next() {
        x;
    } else {
        break;
    }
};

B

let a: Array<u8> = array![1, 2, 3];
let mut iter = a.into_iter();
loop {
    if let Option::Some(x) = iter.next() {
        x * 2;
    } else {
        break;
    }
};

C

let a: Array<u8> = array![1, 2, 3];
for x in a {
    2 * x;
};

A: gas usage est.: 12090
B: gas usage est.: 10290
C: gas usage est.: 9990

There's a overhead with map, and it seems to be linear with the size of the iterator.
Using a RangeIterator of size 30 gives:

A: gas usage est.: 153620
B: gas usage est.: 80870
C: gas usage est.: 83870

However the strengths of iterators is the ability to define mutation in a declarative way, and using composition/chaining with multiple adapters (filter, take, ...), so it's still worthwhile to try to optimize in my opinion.

Let's take a last example where we need to perform two distinct mutation over an array.
With imperative approach, we have to use an intermediate array (supposing that we're not minimizing the two mutations in only one):

A

let mut iter = (0..100_u8)
    .into_iter()
    .map(|x| {
        if (x % 2) == 0 {
            x * 2
        } else {
            x + 1
        }
    })
    .map(|x| x + 1);

let mut collect = array![];
for x in iter {
    collect.append(x);
}

B

let mut iter = (0..100_u8).into_iter();

let mut intermediate_arr = array![];
for x in iter {
    if (x % 2) == 0 {
        intermediate_arr.append(x * 2);
    } else {
        intermediate_arr.append(x + 1);
    }
};

let mut collect = array![];
for x in intermediate_arr {
    collect.append(x + 1);
}

A: gas usage est.: 748030
B: gas usage est.: 747140

Given the map iterator overhead, it is same performance as creating a new intermediate array.
Do you think there could be way to optimize iterator performances?

can you compare instead of B with:

let mut iter = (0..100_u8).into_iter();

let mut intermediate_arr = array![];
for x in iter {
    intermediate_arr.append(if (x % 2) == 0 {
        x * 2
    } else {
        x + 1
    });
};

let mut collect = array![];
for x in intermediate_arr {
    collect.append(x + 1);
}

(a bit more equivalent)

---

corelib/src/ops/function.cairo line 26 at r9 (raw file):

}

/// An implementation of `FnOnce` when `Fn` is implemented and the receiver is by-reference.

i don't know what do you mean "by-reference" in cairo - but in any case - why is this impl required now?

[julio4]

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

---

a discussion (no related file):

Previously, orizi wrote…

can you compare instead of B with:

let mut iter = (0..100_u8).into_iter();

let mut intermediate_arr = array![];
for x in iter {
    intermediate_arr.append(if (x % 2) == 0 {
        x * 2
    } else {
        x + 1
    });
};

let mut collect = array![];
for x in intermediate_arr {
    collect.append(x + 1);
}

(a bit more equivalent)

True, this gives:
B: gas usage est.: 737140

---

corelib/src/ops/function.cairo line 26 at r9 (raw file):

Previously, orizi wrote…

i don't know what do you mean "by-reference" in cairo - but in any case - why is this impl required now?

It was actually by-snapshot and forgot to revert after rebase with #6992
Removed

[orizi]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

True, this gives:
B: gas usage est.: 737140

interesting.
in any case - i don't see any reason not to approve the PR - we should probably find the cause for the degradation, and improve it, (in cairo code, or by an additional new optimization stage) but it is not that extreme just yet.

---

a discussion (no related file):

Previously, orizi wrote…

Adding @TomerStarkware for 2nd eye, and more insights for the FnOnce vs Fn stuff.

ping

[julio4]

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

---

a discussion (no related file):

Previously, orizi wrote…

interesting.
in any case - i don't see any reason not to approve the PR - we should probably find the cause for the degradation, and improve it, (in cairo code, or by an additional new optimization stage) but it is not that extreme just yet.

Any recommendations to correctly profile cairo code and try to find optimizations? Or it would be more at the compiler level?
It's not extreme, but worst case seems to be almost 100% overhead

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

Any recommendations to correctly profile cairo code and try to find optimizations? Or it would be more at the compiler level?
It's not extreme, but worst case seems to be almost 100% overhead

in the particular case of the 100% overhead i also believe that the Map implementation isn't being dropped - the issue there was mostly that in the good case there was no need to actually pass a value around (as it basically just checked if x*2 had an overflow).

so i'll drop the performance issue for now - probably heavier inlining of closures would do a lot of the work.

[orizi]

Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @julio4)

---

corelib/src/iter.cairo line 234 at r11 (raw file):

mod adapters;
mod traits;
pub use traits::{IntoIterator, Iterator};

add endline.

Code quote:

pub use traits::{IntoIterator, Iterator};

[julio4]

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @orizi)

---

corelib/src/iter.cairo line 234 at r11 (raw file):

Previously, orizi wrote…

add endline.

Done.

[orizi]

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)

---

a discussion (no related file):

Previously, orizi wrote…

ping

@TomerStarkware @gilbens-starkware please make sure to have a look.

[TomerStarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @julio4)

---

corelib/src/iter/traits/iterator.cairo line 98 at r12 (raw file):

        +Drop<T>,
        +Drop<F>,
        +Copy<F>,

can we use Fn instead of Fnonce + Copy?

Code quote:

 +Copy<F>,

[julio4]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)

---

corelib/src/iter/traits/iterator.cairo line 98 at r12 (raw file):

Previously, TomerStarkware wrote…

can we use Fn instead of Fnonce + Copy?

Done.

[orizi]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)