Row Data Stored On The Heap

[Gali-StarkWare]

No description provided.

[reviewable-StarkWare]

This change is 

[Gali-StarkWare]

• Row Data Stored On The Heap #1009 👈 (View in Graphite)
• dev

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

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 78.72340% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.86%. Comparing base (81d1fe3) to head (329e0e2).

Files with missing lines	Patch %	Lines
crates/air_utils/src/trace/row_iterator.rs	66.66%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1009      +/-   ##
==========================================
- Coverage   92.88%   92.86%   -0.02%     
==========================================
  Files         105      105              
  Lines       14242    14259      +17     
  Branches    14242    14259      +17     
==========================================
+ Hits        13228    13241      +13     
- Misses        934      938       +4     
  Partials       80       80              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shaharsamocha7]

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

---

a discussion (no related file):
Can we check if this causes degradation in the performance?

[Gali-StarkWare]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

a discussion (no related file):

Previously, shaharsamocha7 wrote…

Can we check if this causes degradation in the performance?

Changed to vector so now compile time is back to normal

[andrewmilson]

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/air_utils/src/trace/row_iterator.rs line 35 at r2 (raw file):

            .v
            .iter_mut()
            .map(|i| unsafe {

This is no longer an index. It's a chunk of a column

Suggestion:

col_chunk

[andrewmilson]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare, @ohad-starkware, and @shaharsamocha7)

---

crates/air_utils/src/trace/row_iterator.rs line 59 at r2 (raw file):

            .v
            .iter_mut()
            .map(|i| unsafe {

Same here

Code quote:

i

---

crates/air_utils/src/trace/row_iterator.rs line 87 at r2 (raw file):

            left[i] = lhs;
            right[i] = rhs;
        }

This doesn't need unsafe

Suggestion:

        let (left, right) = self.data.into_iter().map(|| ...).unzip();
        ...

[shaharsamocha7]

Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)

---

a discussion (no related file):

Previously, Gali-StarkWare wrote…

Changed to vector so now compile time is back to normal

nice, what about the running time?

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @Gali-StarkWare and @ohad-starkware)

---

crates/air_utils/src/trace/component_trace.rs line 111 at r2 (raw file):

                .iter_mut()
                .map(|col| col.as_mut_slice())
                .collect_vec(),

Do we want a collect_vec here? @andrewmilson

Code quote:

collect_vec(),

[Gali-StarkWare]

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

---

a discussion (no related file):

Previously, shaharsamocha7 wrote…

nice, what about the running time?

Looks same as before in the CI, we can do a comparison on the super computer

---

crates/air_utils/src/trace/component_trace.rs line 111 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Do we want a collect_vec here? @andrewmilson

I thought it's always better to use collect_vec instead of collect

---

crates/air_utils/src/trace/row_iterator.rs line 35 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

This is no longer an index. It's a chunk of a column

Done.

---

crates/air_utils/src/trace/row_iterator.rs line 59 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

Same here

Done.

---

crates/air_utils/src/trace/row_iterator.rs line 87 at r2 (raw file):

Previously, andrewmilson (Andrew Milson) wrote…

This doesn't need unsafe

Nice!

[andrewmilson]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @ohad-starkware and @shaharsamocha7)

---

crates/air_utils/src/trace/component_trace.rs line 111 at r2 (raw file):

Previously, Gali-StarkWare wrote…

I thought it's always better to use collect_vec instead of collect

LGTM collect_vec and collect are equivalent here

[Gali-StarkWare]

Reviewed 1 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Dismissed @andrewmilson from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrewmilson, @ohad-starkware, and @shaharsamocha7)

[Gali-StarkWare]

Dismissed @andrewmilson from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ohad-starkware and @shaharsamocha7)

[Gali-StarkWare]

Reviewed 1 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ohad-starkware and @shaharsamocha7)

Anavailable