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

No alloc experiment #42

Closed
wants to merge 7 commits into from
Closed

No alloc experiment #42

wants to merge 7 commits into from

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Dec 19, 2021

This adds support for removing the requirement for alloc from the crate. Following changes were introduced:

  • All serialization returns GenericArray<u8, _> instead of Vec<u8> and requires some where bounds, which shouldn't affect the user unless they are using a generic interface themselves.
  • NonVerifiableClient and VerifiableClient don't hold data anymore, that way the user has to store the data and we don't have to bind our items by lifetimes. This also makes serialization without allocation possible, because we have a known size now. Alternatively we could either introduce a lifetimes to these items, or introduce a generic that can be AsRef<u8> to own the data without allocation. Both would make serialization without alloc support impossible or difficult.
  • batch_evaluate is now only supported with alloc, because I have no clue how to support it without allocating memory to save the generated EvaluationElements.
  • batch_finalize now returns an Iterator instead of a Vec.

So the missing part to make it completely support building without alloc would be to split the batch_evaluate functions into two parts so output can be collected by the user without allocation. This is already done internally, so we can just make the interface public. We can still leave the original batch_evaluate as a convenience function for users that have support for alloc. What do you think @kevinlewi?

This builds on top of #34.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 19, 2021
@daxpedda
Copy link
Contributor Author

Cherry-picked some commits that don't really belong here to #34.

@daxpedda daxpedda force-pushed the no-alloc branch 2 times, most recently from ae6fa39 to 9fee5e1 Compare December 22, 2021 13:00
@daxpedda
Copy link
Contributor Author

Re-based and ready to review, the question remaining in the OP can be handled in a different PR too.

@daxpedda daxpedda marked this pull request as ready for review December 22, 2021 13:01
@daxpedda
Copy link
Contributor Author

daxpedda commented Dec 23, 2021

Re-based. CI is fine, failed on downloading Rust.

@daxpedda
Copy link
Contributor Author

Merged by #44.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants