feat: use fixed number of db transactions for storage proofs #14860
Conversation
Rjected
force-pushed
the
dan/fixed-db-tx-multiproof
branch
4 times, most recently
from
64136eb to
ab4eb0f
Compare
| if self.pending_targets.front().is_none() { | ||
| return; | ||
| } | ||
|
|
||
| let next_input = self.pending_targets.pop_front().unwrap(); | ||
|
|
There was a problem hiding this comment.
can be let Some else return?
Rjected
force-pushed
the
dan/fixed-db-tx-multiproof
branch
5 times, most recently
from
b68a291 to
565b7fc
Compare
Rjected
requested review from
rkrasiuk,
shekhirin,
fgimenez and
gakonst
as code owners
Rjected
force-pushed
the
dan/fixed-db-tx-multiproof
branch
7 times, most recently
from
6018b0c to
2d9dc24
Compare
| // initialize proof task txs | ||
| let mut proof_task_txs = Vec::with_capacity(max_concurrency); | ||
| for item in &mut proof_task_txs { | ||
| let provider_ro = view.provider_ro()?; | ||
| let tx = provider_ro.into_tx(); | ||
| *item = ProofTaskTx::new(tx, task_ctx.clone()); |
There was a problem hiding this comment.
we're currently paying for this upfront before twe spawn this, right?
which doesn't seem reasonable especially because we spawn this and can just do all of this in the background or on demand.
There was a problem hiding this comment.
Yeah, we could do this on the spawned run function
There was a problem hiding this comment.
We now create transactions on-demand
| let RunningProofTask { task_receiver, sender } = running_task; | ||
| match task_receiver.try_recv() { | ||
| Ok(ProofTaskOutput { tx, result }) => { | ||
| let _ = sender.send(result); |
There was a problem hiding this comment.
this feels a bit weird, why are we awaiting both the result and the sender here only to send the result through that sender
There was a problem hiding this comment.
I guess we could send the result directly in the spawned storage proof task, and only pass the tx back in the ProofTaskOutput
There was a problem hiding this comment.
now we send the result directly to the sender in storage_proof
| loop { | ||
| // TODO: condvar for | ||
| // * either running or proof tasks have stuff | ||
| // * otherwise yield thread | ||
| let message = match self.proof_task_rx.try_recv() { | ||
| Ok(message) => match message { |
There was a problem hiding this comment.
hmm, this task now has do advance more than one channel, making this loop a bit complex with try_recv.
isn't the flow of messages just
<- incoming request for storage proof with a tx
if < concurrency: pick available db tx or spawn that task without one and create one on the task
handle the storageproof request on the task and return the db tx
then this only has to loop over a single receiver
or are we then running into issues with generics if we need to carry the db tx in the prooftaskmessage enum
There was a problem hiding this comment.
The design currently uses 2 channels, one for incoming proof requests, and one for returning txs from finished storage proof tasks. If we want only one channel in this loop, we would need another way (other than message passing) to keep track of txs in-use / not in-use
Rjected
force-pushed
the
dan/fixed-db-tx-multiproof
branch
3 times, most recently
from
bac17cc to
d7397bb
Compare
|
Note, I still have this: let max_concurrency = 32;any suggestions on the value / how to derive this? |
Rjected
force-pushed
the
dan/fixed-db-tx-multiproof
branch
from
6a3cd28 to
e039441
Compare
| /// Creates a new [`ProofTaskManager`] with the given max concurrency, creating that number of | ||
| /// cursor factories. | ||
| /// | ||
| /// Returns an error if the consistent view provider fails to create a read-only transaction. |
| /// Spawns the proof task on the executor, with the input multiproof targets. | ||
| /// | ||
| /// If a task cannot be spawned immediately, this will be queued for completion later. |
There was a problem hiding this comment.
this always queues and the spawn happens in the main loop
| let Some(proof_task_tx) = self.get_or_create_tx()? else { return Ok(()) }; | ||
|
|
||
| let Some((pending_proof, sender)) = self.pending_proofs.pop_front() else { | ||
| // if there are no targets to do anything with put the tx back | ||
| self.proof_task_txs.push(proof_task_tx); | ||
| return Ok(()) | ||
| }; |
There was a problem hiding this comment.
should we swap these, so that txs aren't created if they won't be used?
Creates a task that manages a fixed number of db transactions, specifically for calculating many storage proofs
ref #14850