try-runtime: dynamic storage query sizes #13923
Conversation
liamaharon
added
A0-please_review
liamaharon
changed the title
liamaharon
changed the title
|
Switching this to a draft and implementing a dynamic batch size as described here #13246 |
liamaharon
added
A3-in_progress
liamaharon
changed the title
liamaharon
changed the title
liamaharon
added
A0-please_review
There was a problem hiding this comment.
Looks good on the whole.
Could you please also test the difference with the parity nodes? https://gist.github.com/ggwpez/81db110fe4390ed9a7622f5857dfc4ff
I dont expect there to be any - besides it being more fault tolerant.
| } else { | ||
| uri.clone() | ||
| }; | ||
| let http_client = HttpClientBuilder::default().build(uri).map_err(|e| { |
There was a problem hiding this comment.
I compared the speed of loading storage from a local node against master. Performance is identical (+/- <2% between runs)
So why do we use a WS client here formerly @niklasad1?
Is it supposed to be faster?
There was a problem hiding this comment.
Yeah, the websocket client is slightly faster and this is used for instance by the staking miner v1 as well.
I'm still not convinced that throwing extra threads on this will be of any benefit at all, this entire could be be simplified a lot by using FuturesUnordered instead of spawning manual threads for waiting on async I/O
There was a problem hiding this comment.
Inserting data into storage is very cpu heavy so I think it's worth it having a dedicated core working on that.
Otherwise I agree that there's likely little to no improvement adding more threads to do network io. Switching to FuturesUnordered for the network io tasks seems like a great idea we could make as part of a future refactor.
…x-try-runtime-rpc-panic
Co-authored-by: Niklas Adolfsson <[email protected]>
…h/substrate into liam-fix-try-runtime-rpc-panic
Confirmed these are working |
|
Hey @niklasad1, the polkadot staking-miner rpc expects that remote-externalities uses a WsClient: https://github.com/paritytech/polkadot/blob/5fd2bf8c9a0665b361eb26823030a5e0e65459b4/utils/staking-miner/src/rpc.rs#L108 Do you suggest that I refactor the staking-miner and open a companion PR, or try to modify this PR so that remote-externalities supports either a WsClient or HttpClient? |
I think it's fine to use the HTTP client just for the "batch requests/remote externalities" just change https://github.com/paritytech/polkadot/blob/master/utils/staking-miner/src/main.rs#L315-#L323 to take an URL instead (in case it would be a ws:// but you already added converting ws to http URL under the hood) |
liamaharon
added
A4-companion
|
bot merge |
|
Waiting for commit status. |
|
bot merge force |
* improve batch rpc error message * wip aimd storage data fetch * complete aimd function refactor * make batch_request function async * improve function name * fix load_child_remote issue * slight efficiency improvement * improve logs and variable name * remove redundant comment * improve comment * address pr comments * Update utils/frame/remote-externalities/src/lib.rs Co-authored-by: Niklas Adolfsson <[email protected]> * simplify client handling * fix type issue * fix clippy issue * try to trigger ci * try to trigger ci --------- Co-authored-by: Niklas Adolfsson <[email protected]>
Closes #13246
polkadot companion: paritytech/polkadot#7111
Changes
get_storage_data_dynamic_batch_size, allowing users to efficiently process a large amount of jsonrpc payloads without knowing the node max payload size configuration. The function calls itself recursively, increasingbatch_sizewhen the node responds successfully and reducing it when the node returns an error.get_storage_data_dynamic_batch_sizefor fetching storage keys.jsonrpseelibrary such as"961d622f: accumulated message length exceeds maximum"and the node would also randomly refuse to continue servicing WsClient, returning incorrectly formatted errors (missing request IDs) that were also causing more internal errors inside thejsonrpseelibrary. I'm not sure whether this issue is in thejsonrpseelibrary or the way the Substrate RPC is configured or worth this is worth opening an issue to investigate, let me know if I should.rpc_child_get_keys. It runs extremely fast on just one thread (2 seconds on a Macbook Pro), and the logic to run it across multiple threads was quite complex and causing some difficult to debug issues. I've benchmarked this to confirm negligible performance impact, I do not think parallelising this piece of code is worth it at this time.Notes
master. Performance is identical (+/- <2% between runs)wss://rpc.polkadot.io:443. it took 76min on my home internet, upload speed was be far the biggest bottleneck.