Skip to content

feat: reduced number of u128 ops in serialization#155

Closed
zac-williamson wants to merge 1 commit intomainfrom
zw/improve_serialization
Closed

feat: reduced number of u128 ops in serialization#155
zac-williamson wants to merge 1 commit intomainfrom
zw/improve_serialization

Conversation

@zac-williamson
Copy link
Copy Markdown
Collaborator

Description

calling fn from_be_bytes calls a large number of u128 operations as byte sums are being computed. This PR constructs bignum limbs out of byte arrays using Field arithmetic operations, converting the result to u128.

Problem*

u128 ops are expensive and from_be_bytes was using a lot of them, making from_be_bytes cost ~1,000 gates more than neccessary

Summary*

PR changes from_be_bytes to evaluate most of its arithmetic operations using Field arithmetic. This is safe as we are computing sums of u8 objects and implicitly know that the output is range constrained.

Additional Context

This PR should not create any breaking changes.

PR Checklist*

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

@kashbrti
Copy link
Copy Markdown
Contributor

kashbrti commented Apr 4, 2025

Thanks for catching this! benchmarks for from_be_bytes were not implemented, so this went unnoticed. I'll add some benchmarks for the operation in this PR.

@kashbrti
Copy link
Copy Markdown
Contributor

kashbrti commented Apr 4, 2025

noticed we even don't have tests for to_be_bytes

Copy link
Copy Markdown
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.

The beta.4 release of noir will make the most efficient implementation of this to be

limb.assert_max_bit_size::<128>()
result[N - i - 1] = limb;

We should avoid using an unconstrained function to do this.

@kashbrti
Copy link
Copy Markdown
Contributor

kashbrti commented Apr 8, 2025

@zac-williamson @TomAFrench I have another PR open #157, that replicates this PR and also adds some other serialization methods. Should we close this branch and switch to the other PR?

@kashbrti
Copy link
Copy Markdown
Contributor

kashbrti commented Apr 8, 2025

closing this as it's done in #157

@kashbrti kashbrti closed this Apr 8, 2025
@github-project-automation github-project-automation Bot moved this from 👀 To Triage to ✅ Done in Noir Libraries Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants