Serve typed blocks from recent block cache to avoid repeated deserialization#6400
Serve typed blocks from recent block cache to avoid repeated deserialization#6400incrypto32 wants to merge 2 commits intomasterfrom
Conversation
…h crate Move EthereumJsonBlock and JSON patching utilities from chain/ethereum to graph/src/components/ethereum/ so they can be used by the store layer without circular dependencies. This prepares for typed block caching where CachedBlock::from_json() needs access to these utilities.
incrypto32
force-pushed
the
krishna/typed-block-cache
branch
2 times, most recently
from
1f360ae to
fd65e1a
Compare
incrypto32
force-pushed
the
krishna/typed-block-cache
branch
from
fd65e1a to
4ba28d6
Compare
lutter
left a comment
lutter
left a comment
There was a problem hiding this comment.
Nice! I left a few comments for additional improvements - I am totally fine merging this as-is to see how much this helps, but I think we should also do some of the suggestions as follow-on work. In particular, we should be able to get rid of deep clones of blocks; we also have a pretty big number of different block structs all over the code base; it would be nice to simplify and reduce that though that might be pretty hairy.
chain/ethereum/src/chain.rs
Outdated
| BlockStream, BlockStreamBuilder, BlockStreamError, BlockStreamMapper, FirehoseCursor, | ||
| TriggersAdapterWrapper, | ||
| }; | ||
| use graph::components::ethereum::EthereumJsonBlock; |
There was a problem hiding this comment.
Very minor: since this is in a module called ethereum, it's probably enough to call this JsonBlock
|
|
||
| pub fn into_light_block(self) -> LightEthereumBlock { | ||
| match self { | ||
| CachedBlock::Full(block) => block.block.as_ref().clone(), |
There was a problem hiding this comment.
If you change this to
pub fn into_light_block(self) -> Arc<LightEthereumBlock> {
match self {
CachedBlock::Full(block) => block.block,
CachedBlock::Light(block) => Arc::new(block),
}
}you can avoid a deep clone of the block
| .ok() | ||
| }) | ||
| .map(Arc::new) | ||
| .map(|cached| Arc::new(cached.into_light_block())) |
There was a problem hiding this comment.
This then becomes .map(CachedBlock::into_light_block)
|
|
||
| /// Typed cached block for Ethereum. Stores the deserialized block so that | ||
| /// repeated reads from the in-memory cache avoid `serde_json::from_value()`. | ||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
It would be nice to get rid of Clone for this and all other block structs; that's probably a bigger puzzle and requires using Arc judiciously in various data structures, i.e., something for another PR; but conceptually, blocks are readonly and it should therefore be possible to Arc them.
|
|
||
| /// Returns blocks as raw JSON. Used by callers that need the original | ||
| /// JSON representation (e.g., GraphQL block queries, CLI tools). | ||
| async fn blocks_as_json( |
There was a problem hiding this comment.
We should try and get rid of this one, but that's also for another PR
| Ok(ptr) | ||
| } | ||
|
|
||
| fn cache_block_to_block_ptr_ext(block: &CacheBlock) -> Result<ExtendedBlockPtr, Error> { |
There was a problem hiding this comment.
This could be a method on CacheBlock
| let block = JsonBlock::new(block.ptr(), parent_hash, block.data().ok()); | ||
| self.recent_blocks_cache.insert_block(block); | ||
| let json_block = JsonBlock::new(block.ptr(), parent_hash, block.data().ok()); | ||
| self.recent_blocks_cache.insert_json_block(json_block); |
There was a problem hiding this comment.
upsert_block is only used in 2 places (apart from tests) It should be possible to tighten the type of block to a concrete type here so we don't have to go through JSON. It's kinda low priority since this code path is only used on the block ingestor, which isn't that performance critical.
2 participants
No description provided.