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

Simplify debug naming of variables in constraints #435

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

matthiasgoergens
Copy link
Collaborator

@matthiasgoergens matthiasgoergens commented Oct 21, 2024

Instead of passing an FnOnce closure with no arguments, we just pass the name itself.

It looks like we copied the old convention from other R1CS proving systems like bellman. But we always execute our closure, rendering the whole exercise pointless, while they only do execute theirs in debug contexts. (Thanks to Ming for the historic context!)

(And even for bellman, it's unlikely they actually get anything out of the extra complexity: making and passing closures around is a lot of work at runtime. And an optimising compiler that can remove an unused closure can also much easier remove an unused string.)


I ran some benchmarks. The change ever so slightly speeds up compilation and runtime, and decreases the size of our binaries.

On master:

$ du -h target/*/libceno_zkvm.rlib
4.8M    target/debug/libceno_zkvm.rlib
4.4M    target/release/libceno_zkvm.rlib

On this branch:

$ du -h target/*/libceno_zkvm.rlib
4.7M    target/debug/libceno_zkvm.rlib
4.3M    target/release/libceno_zkvm.rlib

This benchmark is just for fun, the PR is for readability.

@matthiasgoergens matthiasgoergens changed the title Simplify query_instance's API Simplify debug naming of variables in constraints Oct 21, 2024
@hero78119
Copy link
Collaborator

hero78119 commented Oct 28, 2024

I would say this change definitely worth to explore as we always looking for improve the readibility.

But honestly wrapping them in || <name> closure vs "pure string" looks not that different to me, but with "closure " one benefit is we keeping the door open for further experimenting skipping variable name string in release build, see bellperson

https://github.com/filecoin-project/bellperson/blob/95fd3fc10e740547b53ce8e86a04c49509af6a41/src/groth16/prover/mod.rs#L104-L115

And we can expect it might be help to make binary size smaller if we gonna distribute prover binary into IOT device.

So I would suggest before clear out the thoughts, please on hold this issue and spend effort on other more urgent milestone task. We definitely look back into this issue sometime, and prove we can achieve same effect without "closure" then it will be super nice

@matthiasgoergens
Copy link
Collaborator Author

matthiasgoergens commented Oct 28, 2024

Closures aren't free to create and pass around, even if you don't use them.

Yes, a clever optimizing compiler can perhaps remove them, but the same techniques would allow the compiler also remove the &str.

And we can expect it might be help to make binary size smaller if we gonna distribute prover binary into IOT device.

Well, the PR branch has about 3% smaller binary sizes than what's currently in master. Exactly because not handling closures is easier for the compiler, I guess.

This is PR mainly for readability, and to avoid our typical cargo culting and tendency to unnecessarily overcomplicate things.

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