Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Try replace callstack with a linked list #6747

Merged
merged 7 commits into from
Dec 11, 2024
Merged

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 9, 2024

Description

Problem*

Resolves

Summary*

Test to see memory usage change from replacing the callstack with a (shared) linked list.

Additional Context

Surprisingly, I had trouble finding an existing crate which provides a shared linked list. I ended up writing my own. (std::collections::LinkedList boxes each item so it isn't shared). Callstacks come up several times in parallel iterators unfortunately so I had to use Arc over Rc.

Still need to reverse the display order of callstacks now that they're stored in reverse order due to being lists.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher jfecher marked this pull request as draft December 9, 2024 20:39
Copy link
Contributor

github-actions bot commented Dec 9, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.423s -4%
regression_4709 0m1.540s -4%
ram_blowup_regression 0m16.817s -3%
private-kernel-tail 0m1.317s 8%
private-kernel-reset 0m9.096s -5%
private-kernel-inner 0m2.356s -12%
parity-root 0m0.940s 6%
noir-contracts 2m36.021s -8%

Copy link
Contributor

github-actions bot commented Dec 9, 2024

Peak Memory Sample

Program Peak Memory %
keccak256 78.17M -4%
workspace 121.79M -1%
regression_4709 294.72M -11%
ram_blowup_regression 2.44G 0%
private-kernel-tail 208.76M -10%
private-kernel-reset 861.44M 100%
private-kernel-inner 307.68M -9%
parity-root 174.42M -13%

@jfecher
Copy link
Contributor Author

jfecher commented Dec 9, 2024

regression_4709 300.69M -10%

Maybe not big enough to warrant merging? Wish we could avoid the Arc, otherwise I'd be more fine with merging.

Also ram_blowup_regression is 2.55G on other PRs but is showing as 0% changed here. Should be roughly -4%

@TomAFrench
Copy link
Member

Let's keep this PR open until #6750 is landed to see what the effects are on the protocol circuits.

@jfecher jfecher mentioned this pull request Dec 10, 2024
5 tasks
@jfecher
Copy link
Contributor Author

jfecher commented Dec 10, 2024

Comparing this to #6753, the peak memory usage reduction is much closer to optimal, -4% and -10% compared to a maximum of -5% and -15% respectively for those tests.

@TomAFrench
Copy link
Member

The report isn't showing this correctly but it's cutting the memory usage of private-kernel-reset by ~400MB.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving but not going to merge as it was left in draft.

@TomAFrench
Copy link
Member

Still need to reverse the display order of callstacks now that they're stored in reverse order due to being lists.

Ah should have read the OP more closely.

@jfecher
Copy link
Contributor Author

jfecher commented Dec 11, 2024

Reversed iteration direction of the call stacks so this should be ready for review / merge now.
Going to see how much this change impacts runtime performance.

@jfecher jfecher marked this pull request as ready for review December 11, 2024 18:09
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TomAFrench TomAFrench added this pull request to the merge queue Dec 11, 2024
Merged via the queue into master with commit f6957fa Dec 11, 2024
62 checks passed
@TomAFrench TomAFrench deleted the jf/linked-list branch December 11, 2024 19:22
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 11, 2024
…g/noir#6747)

chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 11, 2024
chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
self.len == 0
}

fn pop_front(&mut self) -> Option<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both push_back and pop_front operate on the head; I understand what you meant by push_back actually being a "push_front", but wouldn't it be better to name both of these head operations to be the same? I thought pop_front will go to the opposite end from push_back in an O(n) operation. Why not just push and pop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's my mistake. They were originally both *_back to make it more easy for me to migrate existing code, then I changed pop_front to be the front to be more accurate but thought renaming the public push_back to front as well could be confusing to CallStack users who want to add something to the conceptual "back" of the structure, unaware that the structure is essentially reversed internally.

I'd be for renaming it back to pop_back.

@aakoshh
Copy link
Contributor

aakoshh commented Dec 12, 2024

My all time favourite introduction to Rust has some hilarious discussion about linked lists that might explain why there aren't many implementations around: https://rust-unofficial.github.io/too-many-lists/index.html :)

AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 12, 2024
)

chore: Try replace callstack with a linked list (noir-lang/noir#6747)
chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 12, 2024
chore: Try replace callstack with a linked list (noir-lang/noir#6747)
chore: Use `NumericType` not `Type` for casts and numeric constants (noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos (noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report (noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check (noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses (noir-lang/noir#6757)
fix: parser would hand on function type with colon in it (noir-lang/noir#6764)
chore(docs): Update branding (noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory (noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04 (noir-lang/noir#6755)
chore: free memory for silenced warnings early (noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr (noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type (noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}" (noir-lang/noir#6745)
feat: `std::hint::black_box` function. (noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs (noir-lang/noir#6731)
chore: Cleanup unrolling pass (noir-lang/noir#6743)
fix: allow empty loop headers (noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes (noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering (noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent (noir-lang/noir#6694)
feat: Sync from aztec-packages (noir-lang/noir#6730)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 12, 2024
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: several `nargo test` improvements
(noir-lang/noir#6728)
chore: Try replace callstack with a linked list
(noir-lang/noir#6747)
chore: Use `NumericType` not `Type` for casts and numeric constants
(noir-lang/noir#6769)
chore(ci): Extend compiler memory report to external repos
(noir-lang/noir#6768)
chore(ci): Handle external libraries in compilation timing report
(noir-lang/noir#6750)
feat(ssa): Implement missing brillig constraints SSA check
(noir-lang/noir#6658)
fix: Do not merge expressions that contain output witnesses
(noir-lang/noir#6757)
fix: parser would hand on function type with colon in it
(noir-lang/noir#6764)
chore(docs): Update branding
(noir-lang/noir#6759)
feat(cli): Run command on the package closest to the current directory
(noir-lang/noir#6752)
chore: lock CI to use ubuntu 22.04
(noir-lang/noir#6755)
chore: free memory for silenced warnings early
(noir-lang/noir#6748)
chore(stdlib)!: Remove Schnorr
(noir-lang/noir#6749)
fix: Improve type error when indexing a variable of unknown type
(noir-lang/noir#6744)
fix: println("{{}}") was printing "{{}}" instead of "{}"
(noir-lang/noir#6745)
feat: `std::hint::black_box` function.
(noir-lang/noir#6529)
feat(ci): Initial compilation report on test_programs
(noir-lang/noir#6731)
chore: Cleanup unrolling pass
(noir-lang/noir#6743)
fix: allow empty loop headers
(noir-lang/noir#6736)
fix: map entry point indexes after all ssa passes
(noir-lang/noir#6740)
chore: Update url to 2.5.4 (noir-lang/noir#6741)
feat: Order attribute execution by their source ordering
(noir-lang/noir#6326)
feat(test): Check that `nargo::ops::transform_program` is idempotent
(noir-lang/noir#6694)
feat: Sync from aztec-packages
(noir-lang/noir#6730)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: thunkar <[email protected]>
Co-authored-by: maramihali <[email protected]>
Co-authored-by: TomAFrench <[email protected]>
Co-authored-by: aakoshh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants