From 62aa75a46ed82fbfc82e210bbcc1c32aae61d161 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Tue, 1 Mar 2022 17:36:23 +0100 Subject: [PATCH] chain: refactor `process_partial_encoded_chunk_request` method (#6356) Extract two more methods from `process_partial_encoded_chunk_request` which correspond to `if` bodies that used to be in it. This makes each function shorter and thus easier to read especially as in the future more branches will be added to the method. Furthermore, move sending of the message to the method changing `maybe_send_partial_encoded_chunk_response` into method which prepares the response only. This is a pure refactoring with no changes to the behaviour. Issue: https://github.com/near/nearcore/issues/6242 --- chain/chunks/src/lib.rs | 162 +++++++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 68 deletions(-) diff --git a/chain/chunks/src/lib.rs b/chain/chunks/src/lib.rs index c18c3efddb8..4a3cc8fdff3 100644 --- a/chain/chunks/src/lib.rs +++ b/chain/chunks/src/lib.rs @@ -996,80 +996,109 @@ impl ShardsManager { route_back: CryptoHash, chain_store: &mut ChainStore, ) { - debug!(target: "chunks", "Received partial encoded chunk request for {:?}, part_ordinals: {:?}, receipts: {:?}, I'm {:?}", request.chunk_hash.0, request.part_ords, request.tracking_shards, self.me); - - // Check if we have the chunk in our cache - if let Some(entry) = self.encoded_chunks.get(&request.chunk_hash) { - // Create iterators which _might_ contain the requested parts. - let parts_iter = request.part_ords.iter().map(|ord| entry.parts.get(ord).cloned()); - let receipts_iter = request - .tracking_shards - .iter() - .map(|shard_id| entry.receipts.get(shard_id).cloned()); - - // Pass iterators to function which will evaluate them. Since iterators are lazy - // we will clone as few elements as possible before realizing not all are present. - // In the case all are present, the response is sent. - return self.maybe_send_partial_encoded_chunk_response( - request.chunk_hash, - route_back, - parts_iter, - receipts_iter, - ); - // If not in the cache then check the storage + debug!(target: "chunks", "Received partial encoded chunk request for {:?}, part_ordinals: {:?}, shards: {:?}, I'm {:?}", request.chunk_hash.0, request.part_ords, request.tracking_shards, self.me); + + let response = if let Some(entry) = self.encoded_chunks.get(&request.chunk_hash) { + Self::prepare_partial_encoded_chunk_response_from_cache(request, entry) } else if let Ok(partial_chunk) = chain_store.get_partial_chunk(&request.chunk_hash) { - // Index _references_ to the parts we know about by their `part_ord`. Since only - // references are used in this index, we will only clone the requested parts, not - // all of them. - let present_parts: HashMap = - partial_chunk.parts().iter().map(|part| (part.part_ord, part)).collect(); - // Create an iterator which _might_ contain the request parts. Again, we are - // using the laziness of iterators for efficiency. - let parts_iter = - request.part_ords.iter().map(|ord| present_parts.get(ord).map(|x| *x).cloned()); - - // Same process for receipts as above for parts. - let present_receipts: HashMap = partial_chunk - .receipts() - .iter() - .map(|receipt| (receipt.1.to_shard_id, receipt)) - .collect(); - let receipts_iter = request - .tracking_shards - .iter() - .map(|shard_id| present_receipts.get(shard_id).map(|x| *x).cloned()); - - // Pass iterators to function, same as cache case. - return self.maybe_send_partial_encoded_chunk_response( - request.chunk_hash, - route_back, - parts_iter, - receipts_iter, - ); + Self::prepare_partial_encoded_chunk_response_from_partial(request, partial_chunk) + } else { + None }; + + if let Some(response) = response { + self.peer_manager_adapter.do_send(PeerManagerMessageRequest::NetworkRequests( + NetworkRequests::PartialEncodedChunkResponse { route_back, response }, + )) + } } - /// Checks `parts_iter` and `receipts_iter`, if all elements are `Some` then sends - /// a `PartialEncodedChunkResponse`. `parts_iter` is only evaluated up to the first `None` - /// (if any); since iterators are lazy this could save some work if there were any `Some` - /// elements later in the iterator. `receipts_iter` is only evaluated if `part_iter` was - /// completely present. Similarly, `receipts_iter` is only evaluated up to the first `None` - /// if it is evaluated at all. - fn maybe_send_partial_encoded_chunk_response( - &self, + /// Prepares response to a partial encoded chunk request from + /// a corresponding encoded_chunks cache entry. If the entry can satisfy + /// the requests (i.e. contains all necessary parts and shards) the method + /// returns a [`PartialEncodedChunkResponseMsg`] object; otherwise returns + /// `None`. + fn prepare_partial_encoded_chunk_response_from_cache( + request: PartialEncodedChunkRequestMsg, + entry: &EncodedChunksCacheEntry, + ) -> Option { + // Create iterators which _might_ contain the requested parts. + let parts_iter = request.part_ords.iter().map(|ord| entry.parts.get(ord).cloned()); + let receipts_iter = + request.tracking_shards.iter().map(|shard_id| entry.receipts.get(shard_id).cloned()); + + // Pass iterators to function which will evaluate them. Since iterators are lazy + // we will clone as few elements as possible before realizing not all are present. + // In the case all are present, the response is sent. + Self::prepare_partial_encoded_chunk_response_from_iters( + request.chunk_hash, + parts_iter, + receipts_iter, + ) + } + + /// Prepares response to a partial encoded chunk request from + /// a corresponding partial chunk read from the storage. If the partial + /// chunk can satisfy the requests (i.e. contains all necessary parts and + /// shards) the method returns a [`PartialEncodedChunkResponseMsg`] object; + /// otherwise returns `None`. + fn prepare_partial_encoded_chunk_response_from_partial( + request: PartialEncodedChunkRequestMsg, + partial_chunk: &PartialEncodedChunk, + ) -> Option { + // Index _references_ to the parts we know about by their `part_ord`. Since only + // references are used in this index, we will only clone the requested parts, not + // all of them. + let present_parts: HashMap = + partial_chunk.parts().iter().map(|part| (part.part_ord, part)).collect(); + // Create an iterator which _might_ contain the request parts. Again, we are + // using the laziness of iterators for efficiency. + let parts_iter = + request.part_ords.iter().map(|ord| present_parts.get(ord).map(|x| *x).cloned()); + + // Same process for receipts as above for parts. + let present_receipts: HashMap = partial_chunk + .receipts() + .iter() + .map(|receipt| (receipt.1.to_shard_id, receipt)) + .collect(); + let receipts_iter = request + .tracking_shards + .iter() + .map(|shard_id| present_receipts.get(shard_id).map(|x| *x).cloned()); + + // Pass iterators to function, same as cache case. + Self::prepare_partial_encoded_chunk_response_from_iters( + request.chunk_hash, + parts_iter, + receipts_iter, + ) + } + + /// Checks if `parts_iter` and `receipts_iter` contain no `None` elements. + /// It evaluates the iterators only up to the first `None` value (if any); + /// since iterators are lazy this saves some work if there are any `Some` + /// elements later in the iterator. `receipts_iter` is not iterated if + /// `part_iter` contained any `None` values. + /// + /// If there were no `None` elements in either of the iterators, the method + /// returns a [`PartialEncodedChunkResponseMsg`] object; otherwise returns + /// `None`. + fn prepare_partial_encoded_chunk_response_from_iters( chunk_hash: ChunkHash, - route_back: CryptoHash, parts_iter: A, receipts_iter: B, - ) where + ) -> Option + where A: Iterator>, B: Iterator>, { let maybe_known_parts: Option> = parts_iter.collect(); let parts = match maybe_known_parts { None => { - debug!(target:"chunks", "Not responding, some parts are missing"); - return; + debug!(target:"chunks", "Not sending {:?}, some parts are missing", + chunk_hash.0); + return None; } Some(known_parts) => known_parts, }; @@ -1077,17 +1106,14 @@ impl ShardsManager { let maybe_known_receipts: Option> = receipts_iter.collect(); let receipts = match maybe_known_receipts { None => { - debug!(target:"chunks", "Not responding, some receipts are missing"); - return; + debug!(target:"chunks", "Not sending {:?}, some receipts are missing", + chunk_hash.0); + return None; } Some(known_receipts) => known_receipts, }; - let response = PartialEncodedChunkResponseMsg { chunk_hash, parts, receipts }; - - self.peer_manager_adapter.do_send(PeerManagerMessageRequest::NetworkRequests( - NetworkRequests::PartialEncodedChunkResponse { route_back, response }, - )); + Some(PartialEncodedChunkResponseMsg { chunk_hash, parts, receipts }) } pub fn check_chunk_complete(