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

feat: optimize FieldElement::num_bits #7147

Merged
merged 2 commits into from
Feb 20, 2025
Merged

Conversation

asterite
Copy link
Collaborator

Description

Problem

No issue. While sampling a compilation it landed on FieldElement::num_bits so I was wondering if optimizing it would have any visible effect.

Summary

Before this PR num_bits would allocate the bytes in Vec, now I think there would be no allocations.

I didn't try it locally but it was a simple optimization so trying it here to see what CI says.

Additional Context

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.

@asterite asterite changed the title feat(perf): optimize FieldElement::num_bits feat: optimize FieldElement::num_bits Jan 22, 2025
@TomAFrench
Copy link
Member

I don't think would show up in CI benchmarks as all of the allocations would be temporary and the time to count the numbers of bits in a field element would be insignificant when it comes to compilation.

Running heaptrack locally would show the difference, also you could add a criterion benchmark to track this.

@asterite
Copy link
Collaborator Author

Makes sense. I just benchmarked it locally and the method is like 5 times faster but, as you said, the numbers are too low for it to make a difference. So closing :-)

@asterite asterite closed this Jan 22, 2025
@asterite asterite deleted the ab/num-bits-optimization branch January 22, 2025 14:32
@asterite asterite restored the ab/num-bits-optimization branch February 14, 2025 14:12
@asterite asterite reopened this Feb 14, 2025
@asterite asterite force-pushed the ab/num-bits-optimization branch from d34182e to 112ebc0 Compare February 14, 2025 14:23
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 112ebc0 Previous: 8f20392 Ratio
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 62 s 51 s 1.22

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite
Copy link
Collaborator Author

The benchmark said this is about 60% faster. Criterion doesn't show memory usage but the old method allocates memory while this new method doesn't, so I guess that's good too.

@asterite asterite requested a review from a team February 14, 2025 14:47
@TomAFrench TomAFrench added this pull request to the merge queue Feb 20, 2025
Merged via the queue into master with commit 44c35dc Feb 20, 2025
104 checks passed
@TomAFrench TomAFrench deleted the ab/num-bits-optimization branch February 20, 2025 19:12
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 20, 2025
chore: remove `disable_macros` compile option (noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit (noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3) (noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument (noir-lang/noir#7467)
feat(experimental): show macro errors where they happen (noir-lang/noir#7333)
feat: optimize FieldElement::num_bits (noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries (noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation (noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler (noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations (noir-lang/noir#7456)
feat: add native `u128` type (noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines (noir-lang/noir#7444)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 20, 2025
chore: remove `disable_macros` compile option (noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit (noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3) (noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument (noir-lang/noir#7467)
feat(experimental): show macro errors where they happen (noir-lang/noir#7333)
feat: optimize FieldElement::num_bits (noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries (noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation (noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler (noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations (noir-lang/noir#7456)
feat: add native `u128` type (noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines (noir-lang/noir#7444)
TomAFrench added a commit that referenced this pull request Feb 20, 2025
* master: (89 commits)
  chore: bump external pinned commits (#7472)
  chore: remove `disable_macros` compile option (#7468)
  chore(ci): add workflow to automate bumping aztec-packages commit (#7465)
  chore: Release Noir(1.0.0-beta.3) (#7346)
  chore(ci): Missing dash in profiler command argument (#7467)
  feat(experimental): show macro errors where they happen (#7333)
  feat: optimize FieldElement::num_bits (#7147)
  chore(profiler): Docs on profiler command and more complete error reporting (#7436)
  feat(ci): Release noir-inspector in binaries (#7464)
  chore(docs): Noir Profiler external documentation (#7457)
  feat(ci): Publish binaries for noir-profiler (#7443)
  chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (#7458)
  fix: prevent incorrect ACIRgen caused by noop truncations (#7456)
  feat: add native `u128` type (#7301)
  chore: standardize that doc comments on top of statements and expression are allowed but warn (#7450)
  fix: don't let nargo fmt produce multiple trailing newlines (#7444)
  feat(cli): add noir-execute binary (#7384)
  chore!: make `ResolverError::OracleMarkedAsConstrained` into a full error (#7426)
  chore: simplify reports (#7421)
  fix: do not discard negative sign from field literals in comptime interpreter (#7439)
  ...
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 20, 2025
fix: don't panic when shifting too much (noir-lang/noir#7429)
chore: bump external pinned commits (noir-lang/noir#7472)
chore: remove `disable_macros` compile option (noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit (noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3) (noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument (noir-lang/noir#7467)
feat(experimental): show macro errors where they happen (noir-lang/noir#7333)
feat: optimize FieldElement::num_bits (noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries (noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation (noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler (noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations (noir-lang/noir#7456)
feat: add native `u128` type (noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines (noir-lang/noir#7444)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 21, 2025
fix: don't panic when shifting too much (noir-lang/noir#7429)
chore: bump external pinned commits (noir-lang/noir#7472)
chore: remove `disable_macros` compile option (noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit (noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3) (noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument (noir-lang/noir#7467)
feat(experimental): show macro errors where they happen (noir-lang/noir#7333)
feat: optimize FieldElement::num_bits (noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries (noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation (noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler (noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations (noir-lang/noir#7456)
feat: add native `u128` type (noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines (noir-lang/noir#7444)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 21, 2025
fix: don't panic when shifting too much (noir-lang/noir#7429)
chore: bump external pinned commits (noir-lang/noir#7472)
chore: remove `disable_macros` compile option (noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit (noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3) (noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument (noir-lang/noir#7467)
feat(experimental): show macro errors where they happen (noir-lang/noir#7333)
feat: optimize FieldElement::num_bits (noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries (noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation (noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler (noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs (noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations (noir-lang/noir#7456)
feat: add native `u128` type (noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines (noir-lang/noir#7444)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 21, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: Sync from aztec-packages
(noir-lang/noir#7474)
fix: don't panic when shifting too much
(noir-lang/noir#7429)
chore: bump external pinned commits
(noir-lang/noir#7472)
chore: remove `disable_macros` compile option
(noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit
(noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3)
(noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument
(noir-lang/noir#7467)
feat(experimental): show macro errors where they happen
(noir-lang/noir#7333)
feat: optimize FieldElement::num_bits
(noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error
reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries
(noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation
(noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler
(noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs
(noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations
(noir-lang/noir#7456)
feat: add native `u128` type
(noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression
are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines
(noir-lang/noir#7444)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <[email protected]>
AztecBot added a commit to AztecProtocol/aztec-nr that referenced this pull request Feb 22, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: Sync from aztec-packages
(noir-lang/noir#7474)
fix: don't panic when shifting too much
(noir-lang/noir#7429)
chore: bump external pinned commits
(noir-lang/noir#7472)
chore: remove `disable_macros` compile option
(noir-lang/noir#7468)
chore(ci): add workflow to automate bumping aztec-packages commit
(noir-lang/noir#7465)
chore: Release Noir(1.0.0-beta.3)
(noir-lang/noir#7346)
chore(ci): Missing dash in profiler command argument
(noir-lang/noir#7467)
feat(experimental): show macro errors where they happen
(noir-lang/noir#7333)
feat: optimize FieldElement::num_bits
(noir-lang/noir#7147)
chore(profiler): Docs on profiler command and more complete error
reporting (noir-lang/noir#7436)
feat(ci): Release noir-inspector in binaries
(noir-lang/noir#7464)
chore(docs): Noir Profiler external documentation
(noir-lang/noir#7457)
feat(ci): Publish binaries for noir-profiler
(noir-lang/noir#7443)
chore: Copy #7387 docs into v1.0.0-beta.2 versioned_docs
(noir-lang/noir#7458)
fix: prevent incorrect ACIRgen caused by noop truncations
(noir-lang/noir#7456)
feat: add native `u128` type
(noir-lang/noir#7301)
chore: standardize that doc comments on top of statements and expression
are allowed but warn (noir-lang/noir#7450)
fix: don't let nargo fmt produce multiple trailing newlines
(noir-lang/noir#7444)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <[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.

2 participants