Skip to content

perf(crypto): avoid some unnecessary clones#10014

Merged
pierugo-dfinity merged 17 commits intomasterfrom
pierugo/unnecessary-clones-arc-mutexes
Apr 29, 2026
Merged

perf(crypto): avoid some unnecessary clones#10014
pierugo-dfinity merged 17 commits intomasterfrom
pierugo/unnecessary-clones-arc-mutexes

Conversation

@pierugo-dfinity
Copy link
Copy Markdown
Contributor

@pierugo-dfinity pierugo-dfinity commented Apr 24, 2026

I recommend reviewing by commit:

  • d25992f
    • payload_verifier.rs: avoid clones for which we only need borrows anyways
    • batch_delivery.rs: avoid cloning the internal transcripts as we do not need ownership when converting to protobuf.
  • c48f195
    • Avoid cloning dealings as we can instead pass ownership.
  • Half of the more "invasive" changes: 90e517a
    • Change the interface of NiDkgAlgorithm::create_transcript to now take ownership of the dealings which allows to avoid a second clone down the stack. This comes for free from a Consensus perspective.
  • Other half of the more invasive changes: 4ad048b
    • Similarly, change the interface of IDkgProtocol::create_transcript to now take ownership of the dealings which allows to avoid cloning them and the commitments down the stack. This also positively affects verify_transcript. This also comes for free from Consensus.

@github-actions github-actions Bot added the perf label Apr 24, 2026
@pierugo-dfinity pierugo-dfinity force-pushed the pierugo/unnecessary-clones-arc-mutexes branch from 2cb340a to 90e517a Compare April 24, 2026 16:40
@pierugo-dfinity pierugo-dfinity changed the title perf: avoid some unnecessary clones and Arc<Mutex<_>>s perf: avoid some unnecessary clones Apr 24, 2026
@pierugo-dfinity pierugo-dfinity changed the title perf: avoid some unnecessary clones perf(crypto): avoid some unnecessary clones Apr 24, 2026
@pierugo-dfinity pierugo-dfinity marked this pull request as ready for review April 24, 2026 18:25
@pierugo-dfinity pierugo-dfinity requested a review from a team as a code owner April 24, 2026 18:25
Copy link
Copy Markdown
Contributor

@randombit randombit left a comment

Choose a reason for hiding this comment

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

Looks good to me thanks @pierugo-dfinity

PS this means you're now in charge of NIDKG, take care of her! :)

Copy link
Copy Markdown
Contributor

@eichhorl eichhorl left a comment

Choose a reason for hiding this comment

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

Very nice!

Comment thread rs/consensus/src/consensus/batch_delivery.rs Outdated
Comment thread rs/consensus/dkg/src/payload_builder.rs Outdated
Comment thread rs/crypto/benches/idkg.rs Outdated
Comment thread rs/crypto/benches/ni_dkg.rs
@kpop-dfinity
Copy link
Copy Markdown
Contributor

Maybe we should enable redundant_clone and needless_pass_by_value clippy lints (at least in the CON owned crates). Some of these cloned you're removing should be caught by them

@pierugo-dfinity pierugo-dfinity added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label Apr 27, 2026
@pierugo-dfinity
Copy link
Copy Markdown
Contributor Author

@eichhorl PTAnotherL, I also adapted the transcript creation in data blocks after merging master.

Comment thread rs/consensus/dkg/src/payload_builder.rs Outdated
@pierugo-dfinity pierugo-dfinity added this pull request to the merge queue Apr 29, 2026
Merged via the queue into master with commit d64d610 Apr 29, 2026
37 checks passed
@pierugo-dfinity pierugo-dfinity deleted the pierugo/unnecessary-clones-arc-mutexes branch April 29, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants