refactor(verifier): Remove parallelism #344
Conversation
|
This PR's branch, without parallelism:
Removing parallelism makes verification faster, because of the overhead of rayon thread-handling. |
triton-vm/src/fri.rs
Outdated
| @@ -166,7 +166,7 @@ impl ProverRound { | |||
| } | |||
|
|
|||
| fn merkle_tree_from_codeword(codeword: &[XFieldElement]) -> ProverResult<MerkleTree> { | |||
| let digests = codeword_as_digests(codeword); | |||
| let digests: Vec<_> = codeword.par_iter().map(|&xfe| xfe.into()).collect(); | |||
| MerkleTree::new::<CpuParallel>(&digests).map_err(FriProvingError::MerkleTreeError) | |||
There was a problem hiding this comment.
We will need a single-threaded Merkle tree builder too, no?
There was a problem hiding this comment.
The line you highlight is in the prover. I have removed the parallelism from the function codeword_as_digests and inlined the parallel version that is used in the prover.
There was a problem hiding this comment.
Oh, I think I misunderstood your comment. Let me check the code again.
There was a problem hiding this comment.
You are absolutely right. The FRI verifier is building a Merkle tree from the last round's codeword, and the only logic for building Merkle trees we currently have is using parallelism.1
As a consequence, we seem to require a new Merkle tree builder, CpuSequential. Would you agree?
Footnotes
-
Getting into the nitty gritty details, upon which the FRI verifier should absolutely not depend, Merkle trees with fewer than 512 leafs are built in sequence (unless a rather specific environment variable overrides the default). ↩
There was a problem hiding this comment.
we seem to require a new Merkle tree builder, CpuSequential. Would you agree?
Yes please!
I also noticed one place in some guesser where we use that. Since it's only 8 leafs there, it probably defaults to sequential anyway. It would be good to call the right method directly though, and not have to rely on contextual knowledge.
There was a problem hiding this comment.
Also: it sounds as though both prover and verifier call merkle_tree_from_codeword but the second should call a new merkle_tree_from_codeword_sequential. Correct? Just being explicit to avoid confusion.
There was a problem hiding this comment.
It seems that the code for FRI is not as deduplicated as it could be; merkle_tree_from_codeword is a function associated with ProverRound, and is never called in the verifier, even though the verifier performs the same operations in its method last_round_codeword_merkle_root. Thanks for the heads up, though!
|
Blocked: waiting for twenty-first v0.45.0. |
The parallelism in Triton VM's verifier has more overhead than benefit.
jan-ferdinand
force-pushed
the
remove_parallelism_from_verify_pr
branch
from
53fd6d5 to
baf5860
Compare
|
Merged in baf5860 |
A downstream consumer of Triton VM uses the verifier in an asynchronous context while running its own
rayonthreadpool. This, in combination with Triton VM's use ofrayon, is suspected to cause odd behavior.Remove parallelism through explicit use of
rayonfrom the Triton VM verifier, with the option to re-introduce it when the underlying issues are better understood.