Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up request buffered hashes #6318

Merged
merged 87 commits into from
Feb 13, 2024
Merged

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 1, 2024

closes #6148. closes #6308.

speeds up requesting buffered hashes by

- [x] dividing hashes store (unknown_hashes, buffered_hashes[ and meta]) into eth68 and eth66. this means only hashes will be traversed that can be included in the request being assembled in fill_request_from_buffer_for_peer.

  • improve performance and maintainability at cost of allocating memory for metadata for all hashes, regardless if seen in eth66 announcements only and hence have no metadata.
  • use the transactions list on Peer type to search buffered_hashes after pop_any_idle_peer returns. that cache has capacity 10 240 and buffered hashes has capacity 25 600. also, then we just have to search buffered hashes, and not nested lists (even if they just default to 3 elements long) in buffered hashes for our peer returned by pop_any_idle_peer. effectivity of this is up to how well we can update the transactions list in the Peer type on-op. we will only update the transactions list on Peer type when we need to touch it anyway, on-op. we won't move around elements in lists of peer's seen transactions. it serves only as a hint to which hash cannot be pending, not as a perfect list of which hashes are pending. this totally satisfies requirements and otherwise it doesn't scale.

@emhane emhane marked this pull request as draft February 1, 2024 04:11
@mattsse
Copy link
Collaborator

mattsse commented Feb 1, 2024

this is still very expensive because we blindly iterate all hashes in the hope the peer is the fallback, can we not keep track of the peer's hashes instead when registering them as fallback peers?

@emhane
Copy link
Member Author

emhane commented Feb 1, 2024

this is still very expensive because we blindly iterate all hashes in the hope the peer is the fallback, can we not keep track of the peer's hashes instead when registering them as fallback peers?

yes, this is step 2 as mentioned above. the peer's hashes are already tracked in Peer type. possibly we add a new list which is a subset of Peer { transactions, .. }. two new lists to be precise, since storing them eth66 and eth68 in same list is counterproductive.

@emhane emhane marked this pull request as ready for review February 2, 2024 03:37
@emhane
Copy link
Member Author

emhane commented Feb 3, 2024

this is still very expensive because we blindly iterate all hashes in the hope the peer is the fallback, can we not keep track of the peer's hashes instead when registering them as fallback peers?

we don't even need to register any peer as fallback peer in that case, it's just double up storage of same data that can be derived by checking peer's seen txns against buffered hashes (buffered as in buffered for re-fetch or for first fetch if the hash didn't fit in the request that was triggered by processing the announcement in which the hash was seen the first time).

@emhane emhane added A-networking Related to networking in general A-devp2p Related to the Ethereum P2P protocol labels Feb 10, 2024
@emhane
Copy link
Member Author

emhane commented Feb 10, 2024

This is how the branch is looking on mainnet rn.

Screenshot 2024-02-11 at 00 31 26 Screenshot 2024-02-11 at 00 31 36

This is samply at commit 739d3c1
https://share.firefox.dev/3STlLDb

Will update with new samply from latest commit later.

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

Will do a more comprehensive review soon, but I just saw these constant names and wanted to see if they could be made shorter

Comment on lines +139 to +154
/// Default soft limit for the number of hashes in a
/// [`GetPooledTransactions`](reth_eth_wire::GetPooledTransactions) request, when it is filled
/// from hashes pending fetch. Default is half of the
/// [`SOFT_LIMIT_COUNT_HASHES_IN_GET_POOLED_TRANSACTIONS_REQUEST`] which by spec is 256
/// hashes, so 128 hashes.
pub const DEFAULT_SOFT_LIMIT_COUNT_HASHES_IN_GET_POOLED_TRANSACTIONS_REQUEST_ON_FETCH_PENDING_HASHES:
usize = SOFT_LIMIT_COUNT_HASHES_IN_GET_POOLED_TRANSACTIONS_REQUEST / 2;

/// Default soft limit for a [`PooledTransactions`](reth_eth_wire::PooledTransactions) response
/// when it's used as expected response in calibrating the filling of a
/// [`GetPooledTransactions`](reth_eth_wire::GetPooledTransactions) request, when the request
/// is filled from hashes pending fetch. Default is half of
/// [`DEFAULT_SOFT_LIMIT_BYTE_SIZE_POOLED_TRANSACTIONS_RESPONSE_ON_PACK_GET_POOLED_TRANSACTIONS_REQUEST`],
/// which defaults to 128 KiB, so 64 KiB.
pub const DEFAULT_SOFT_LIMIT_BYTE_SIZE_POOLED_TRANSACTIONS_RESPONSE_ON_FETCH_PENDING_HASHES:
usize = DEFAULT_SOFT_LIMIT_BYTE_SIZE_POOLED_TRANSACTIONS_RESPONSE_ON_PACK_GET_POOLED_TRANSACTIONS_REQUEST / 2;
Copy link
Member

Choose a reason for hiding this comment

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

I like the documentation on these but these names are gigantic, is there any way to make this more concise?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I know they are, but not having enough info can cost a lot of time since then they are interpreted wrong. we already had this with some other constants recently. will eventually change some of these to const functions anyway, then we can asses the lengths again.

@emhane emhane requested review from onbjerg and mattsse February 13, 2024 18:17
Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sharing @Rjected view that some names are gigantic, but can bikeshed separately.

lgtm, nice work

@emhane emhane added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit c0f3d38 Feb 13, 2024
29 checks passed
@emhane emhane deleted the emhane/speed-up-request-buffered-hashes branch February 13, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devp2p Related to the Ethereum P2P protocol A-networking Related to networking in general A-tx-pool Related to the transaction mempool C-perf A change motivated by improving speed, memory usage or disk footprint M-changelog This change should be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve exit condition of pack_eth68 versioned TxHash wrapper, type safety in TransactionFetcher
5 participants