perf: avoid temporary Vec allocations when computing commitments #939

mina86 · 2023-11-01T05:34:04Z

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2023-11-01T05:41:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (599644c) 67.73% compared to head (670c4b2) 67.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
+ Coverage   67.73%   67.80%   +0.07%     
==========================================
  Files         130      130              
  Lines       16397    16418      +21     
==========================================
+ Hits        11106    11133      +27     
+ Misses       5291     5285       -6

Files	Coverage Δ
crates/ibc/src/core/ics04_channel/commitment.rs	`81.48% <100.00%> (+16.48%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Farhad-Shabani

Thanks @mina86! Left a few suggestions.
Also, for our context, appreciate let us know what led to this change? Have you established any kind of benchmarks or obtained a preliminary estimate of the impact this change will have on your project's performance?

crates/ibc/src/core/ics04_channel/commitment.rs

Farhad-Shabani · 2023-11-02T17:04:03Z

crates/ibc/src/core/ics04_channel/commitment.rs

+    sha2::Sha256::digest(data).to_vec()
+}
+
+#[test]


As for the tests, I’d like to have @rnbguy's input!

The test here was purely to make sure that the output doesn’t change during refactoring. I can split this PR into two (one adding the test and the other doing the refactoring) if that’d help to demonstrate it.

@mina86 can you please put the tests under

#[cfg(test)] mod test { ... }

and, make two separate tests for compute_packet_commitment and compute_ack_commitment?

mina86 · 2023-11-02T17:14:10Z

No, this was purely from looking at the code. I’ve done no benchmarks on it.

The reason why I was looking at the code was because I needed to make sure commitments are always 32-byte long.

By the way, ideally I’d like commitment to be struct PacketCommitment([u8; 32]); but that’s a completely different conversation (or struct PacketCommitment(Box<[u8; 32]>);; point is to have it fixed-size).

Farhad-Shabani · 2023-11-02T17:24:21Z

No, this was purely from looking at the code. I’ve done no benchmarks on it.
The reason why I was looking at the code was because I needed to make sure commitments are always 32-byte long.

Alright, Gotcha.

By the way, ideally I’d like commitment to be struct PacketCommitment([u8; 32]); but that’s a completely different conversation (or struct PacketCommitment(Box<[u8; 32]>);; point is to have it fixed-size).

I see where you are coming from. Exactly, this is a totally different topic and a critical one. Better to surface it later when we have enough bandwidth to fully evaluate the changes around.

rnbguy

Great PR, @mina86 ! 🙏 Just requested some changes.

crates/ibc/src/core/ics04_channel/commitment.rs

rnbguy · 2023-11-02T17:42:52Z

crates/ibc/src/core/ics04_channel/commitment.rs

+    sha2::Sha256::digest(data).to_vec()
+}
+
+#[test]


@mina86 can you please put the tests under

#[cfg(test)] mod test { ... }

and, make two separate tests for compute_packet_commitment and compute_ack_commitment?

crates/ibc/src/core/ics04_channel/commitment.rs

.changelog/unreleased/improvements/939-less-alloc.md

crates/ibc/src/core/ics04_channel/commitment.rs

Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]>

rnbguy

Awesome! Thanks @mina86 🤝

* perf: avoid temporary Vec allocations when computing commitments * review * Update crates/ibc/src/core/ics04_channel/commitment.rs Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]> * Update crates/ibc/src/core/ics04_channel/commitment.rs Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]> * Update crates/ibc/src/core/ics04_channel/commitment.rs Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]> * review * Update .changelog/unreleased/improvements/939-less-alloc.md Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]> --------- Signed-off-by: Michal Nazarewicz <[email protected]> Co-authored-by: Rano | Ranadeep <[email protected]>

mina86 force-pushed the a branch from d82a0ea to 0f8fa9a Compare November 1, 2023 05:35

mina86 force-pushed the a branch 2 times, most recently from 6a6c0f9 to e54a3a4 Compare November 1, 2023 06:32

perf: avoid temporary Vec allocations when computing commitments

5da70de

mina86 force-pushed the a branch from e54a3a4 to 5da70de Compare November 1, 2023 06:54

Farhad-Shabani reviewed Nov 2, 2023

View reviewed changes

Farhad-Shabani added the O: optimization Objective: aims to optimize performance, allocations and computations label Nov 2, 2023

mina86 added 2 commits November 2, 2023 18:12

review

4a4e515

Merge remote-tracking branch 'up/main' into HEAD

2fa8469

rnbguy requested changes Nov 2, 2023

View reviewed changes

mina86 and others added 5 commits November 2, 2023 19:45

Update crates/ibc/src/core/ics04_channel/commitment.rs

cd04eb5

Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]>

Update crates/ibc/src/core/ics04_channel/commitment.rs

981f9b6

Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]>

Update crates/ibc/src/core/ics04_channel/commitment.rs

34f07da

Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]>

review

53663b1

Update .changelog/unreleased/improvements/939-less-alloc.md

670c4b2

Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Michal Nazarewicz <[email protected]>

rnbguy approved these changes Nov 2, 2023

View reviewed changes

Farhad-Shabani merged commit ccca3ff into cosmos:main Nov 2, 2023
12 checks passed

Farhad-Shabani mentioned this pull request Nov 2, 2023

Tracks ibc-rs PR #939, #941, #949 informalsystems/basecoin-rs#143

Merged

1 task

mina86 deleted the a branch November 2, 2023 20:01

Farhad-Shabani mentioned this pull request Nov 3, 2023

Continue assisting users in adopting ibc-rs (Q4 2023) #882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: avoid temporary Vec allocations when computing commitments #939

perf: avoid temporary Vec allocations when computing commitments #939

mina86 commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

Farhad-Shabani left a comment

Farhad-Shabani Nov 2, 2023

mina86 Nov 2, 2023 •

edited

Loading

rnbguy Nov 2, 2023

mina86 commented Nov 2, 2023 •

edited

Loading

Farhad-Shabani commented Nov 2, 2023

rnbguy left a comment

rnbguy Nov 2, 2023

rnbguy left a comment

perf: avoid temporary Vec allocations when computing commitments #939

perf: avoid temporary Vec allocations when computing commitments #939

Conversation

mina86 commented Nov 1, 2023 • edited Loading

PR author checklist:

Reviewer checklist:

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Farhad-Shabani Nov 2, 2023

Choose a reason for hiding this comment

mina86 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

rnbguy Nov 2, 2023

Choose a reason for hiding this comment

mina86 commented Nov 2, 2023 • edited Loading

Farhad-Shabani commented Nov 2, 2023

rnbguy left a comment

Choose a reason for hiding this comment

rnbguy Nov 2, 2023

Choose a reason for hiding this comment

rnbguy left a comment

Choose a reason for hiding this comment

mina86 commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

mina86 Nov 2, 2023 •

edited

Loading

mina86 commented Nov 2, 2023 •

edited

Loading