refactor: store `ShardChunk` and `EncodedShardChunk` in the same type by akhi3030 · Pull Request #13359 · near/nearcore

akhi3030 · 2025-04-12T12:39:36Z

The goal of this PR is to create ShardChunk and EncodedShardChunk at the same time which helps with skipping an expensive decoding of TransactionReciept.

Based on the feedback received, this PR introduces a new type that stores both ShardChunk and EncodedShardChunk together so that we can be confident that they are related and create them at the same time.

codecov · 2025-04-12T12:49:27Z

Codecov Report

Attention: Patch coverage is 92.85714% with 12 lines in your changes missing coverage. Please review.

Project coverage is 69.63%. Comparing base (85ced08) to head (f76fead).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
chain/chunks/src/shards_manager_actor.rs	55.55%	2 Missing and 2 partials ⚠️
core/primitives/src/sharding.rs	94.93%	3 Missing and 1 partial ⚠️
chain/client/src/client.rs	89.28%	2 Missing and 1 partial ⚠️
chain/chunks/src/logic.rs	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13359      +/-   ##
==========================================
+ Coverage   69.61%   69.63%   +0.01%     
==========================================
  Files         858      858              
  Lines      170742   170767      +25     
  Branches   170742   170767      +25     
==========================================
+ Hits       118865   118910      +45     
+ Misses      47064    47048      -16     
+ Partials     4813     4809       -4

Flag	Coverage Δ
pytests	`1.54% <0.00%> (-0.01%)`	⬇️
pytests-nightly	`1.61% <0.00%> (-0.01%)`	⬇️
unittests	`69.24% <92.85%> (+0.02%)`	⬆️
unittests-nightly	`69.11% <91.07%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ssavenko-near · 2025-04-22T09:05:27Z

                chunk_extra.validator_proposals().collect(),
                prepared_transactions.transactions,
-                outgoing_receipts,
+                outgoing_receipts.clone(),


We wouldn't need to add this clone() if the outgoing_receipts were returned from the functions as before. Why did you remove it from returned tuple?

It seems the only usage afterwards is for logging the len of outgoing receipts so you can consider just storing that or moving the logs elsewhere.

Before: the function was not creating ShardChunk. It was only "reading" from outgoing_receipts. It still had to be passed by value and returned because of the way encoding and decoding worked.

Now: We are creating ShardChunk and we are storing outgoing_receipts in it. So if we still return it, then we would have to return a clone. So it is cleaner for the caller to send a clone to the function which consumes it.

If you follow how ProduceChunkResult is used, it seems like it is using outgoing_receipts in all sorts of places.

Note that this is not introducing a new clone. In the old code, we were still cloning when we created a ShardChunk.

ssavenko-near · 2025-04-22T09:10:35Z

    ) -> Result<Option<(ShardChunk, PartialEncodedChunk)>, Error> {
        match self.check_chunk_complete(&mut encoded_chunk) {
            ChunkStatus::Complete(merkle_paths) => {
+                let shard_chunk = encoded_chunk.decode_chunk()?;


We're decoding the chunk here and check for validity of decoding in decode_encoded_chunk() called afterwards. That looks fragile. Shall we pull the validity check from within that function to this level as well?

Yeah, I think we should have strong guarantee that the chunk is derived from the encoded chunk. newtype perhaps?

I tried to verify stuff in EncodedAndShardChunk::new() but due to how the crates are setup, I could not. I have moved it closer to EncodedAndShardChunk::new() so the new situation is as good as it was in the past. We could maybe figure out a way to improve things still though.

wacban

I think the code should be restructured first - see comments. Otherwise looks good.

For my info what's the relation between EncodedChunk and PartialEncodedChunk?

wacban · 2025-04-22T12:52:23Z

    ) -> Result<Option<(ShardChunk, PartialEncodedChunk)>, Error> {
        match self.check_chunk_complete(&mut encoded_chunk) {
            ChunkStatus::Complete(merkle_paths) => {
+                let shard_chunk = encoded_chunk.decode_chunk()?;


Yeah, I think we should have strong guarantee that the chunk is derived from the encoded chunk. newtype perhaps?

wacban · 2025-04-22T12:55:42Z

                chunk_extra.validator_proposals().collect(),
                prepared_transactions.transactions,
-                outgoing_receipts,
+                outgoing_receipts.clone(),


It seems the only usage afterwards is for logging the len of outgoing receipts so you can consider just storing that or moving the logs elsewhere.

Longarithm · 2025-04-24T01:25:36Z

        encoded_chunk: EncodedShardChunk,
+        shard_chunk: ShardChunk,


The change makes a lot of sense, we also see a need in this from tracing side #core/performance-optimization > traces analysis of overloaded network @ 💬

However, it feels like there is some duplication here - EncodedShardChunk and ShardChunk represent the same data, just in bytes/structured format.

Maybe there is some way to represent all usecases in a single struct? For example,

struct AnyShardChunk { /// Structured data. Is Some if we produced or reconstructed this chunk chunk: Option<ShardChunk>, /// Raw bytes. Is Some if we decoded or received this chunk bytes: Option<EncodedShardChunk>, } impl AnyShardChunk { /// Constructor - may have debug assertion that chunk matches bytes if both are Some. pub fn new(chunk, bytes) { ... } /// Sets bytes if chunk is set fn decode(&mut self) { ... } /// Sets chunk if bytes is set fn encode(&mut self) { ... } /// Gets chunk, maybe calls encode pub fn chunk(&mut self) { ... } /// Gets bytes, maybe calls decode pub fn bytes(&mut self) { ... } }

It would allow to simplify test setups, where we don't care about performance that much.

thanks for the suggestion, I ended up introducing a new type similar to what you proposed.

akhi3030 · 2025-04-30T18:37:24Z

I am going to make significant changes to this PR based on the feedback above. Marking it as draft till it is ready for another round of reviews.

akhi3030 · 2025-05-01T15:05:58Z

 }
+
+#[derive(Clone)]
+pub struct EncodedAndShardChunk {


I would love to get a suggestion for a better name here.

gpt suggested ShardChunkWithEncoding. Not perfect but works for me.
Also I think it is worth small comment, like "Used to pass chunk around and skip costly encoding/decoding if needed".

Longarithm

Thank you! Looks clean, and it is great that it even reduces LOC and optimises performance together.

Longarithm · 2025-05-01T20:53:27Z

 }
+
+#[derive(Clone)]
+pub struct EncodedAndShardChunk {


gpt suggested ShardChunkWithEncoding. Not perfect but works for me.
Also I think it is worth small comment, like "Used to pass chunk around and skip costly encoding/decoding if needed".

…#13359) The goal of this PR is to create `ShardChunk` and `EncodedShardChunk` at the same time which helps with skipping an expensive decoding of `TransactionReciept`. Based on the feedback received, this PR introduces a new type that stores both `ShardChunk` and `EncodedShardChunk` together so that we can be confident that they are related and create them at the same time.

akhi3030 force-pushed the do-not-encode-decode branch 9 times, most recently from c468753 to 8434800 Compare April 14, 2025 12:11

akhi3030 changed the title ~~wip~~ refactor: create ShardChunk at the same time as EncodedShardChunk Apr 14, 2025

akhi3030 force-pushed the do-not-encode-decode branch 5 times, most recently from f890bc8 to 01137c6 Compare April 19, 2025 17:26

wip

1f4c082

akhi3030 force-pushed the do-not-encode-decode branch from 01137c6 to 1f4c082 Compare April 20, 2025 12:36

akhi3030 marked this pull request as ready for review April 20, 2025 12:49

akhi3030 requested a review from a team as a code owner April 20, 2025 12:49

akhi3030 requested review from ssavenko-near and wacban April 20, 2025 12:49

ssavenko-near approved these changes Apr 22, 2025

View reviewed changes

wacban reviewed Apr 22, 2025

View reviewed changes

Longarithm reviewed Apr 24, 2025

View reviewed changes

akhi3030 added 2 commits April 30, 2025 15:07

Merge branch 'master' into do-not-encode-decode

10acf0c

fix broken merge commit

543274b

akhi3030 marked this pull request as draft April 30, 2025 18:37

akhi3030 force-pushed the do-not-encode-decode branch from db4ef78 to 7bf274b Compare April 30, 2025 18:47

akhi3030 force-pushed the do-not-encode-decode branch 7 times, most recently from f646bc3 to 53cd95a Compare May 1, 2025 13:38

intro new type

51a6913

akhi3030 force-pushed the do-not-encode-decode branch from 53cd95a to 51a6913 Compare May 1, 2025 13:41

akhi3030 changed the title ~~refactor: create ShardChunk at the same time as EncodedShardChunk~~ refactor: store ShardChunk and EncodedShardChunk in the same type May 1, 2025

akhi3030 commented May 1, 2025

View reviewed changes

akhi3030 marked this pull request as ready for review May 1, 2025 15:13

akhi3030 requested review from Longarithm, ssavenko-near and wacban May 1, 2025 15:15

Merge branch 'master' into do-not-encode-decode

213ae84

Longarithm approved these changes May 1, 2025

View reviewed changes

akhi3030 added 2 commits May 2, 2025 09:49

review comment

d21d8c5

rename struct

f76fead

akhi3030 added this pull request to the merge queue May 2, 2025

Merged via the queue into master with commit cf33699 May 2, 2025
27 of 28 checks passed

akhi3030 deleted the do-not-encode-decode branch May 2, 2025 13:13

Conversation

akhi3030 commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wacban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Longarithm Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhi3030 commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Longarithm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akhi3030 commented Apr 12, 2025 •

edited

Loading

codecov bot commented Apr 12, 2025 •

edited

Loading

Longarithm Apr 24, 2025 •

edited

Loading