Skip to content

Conversation

@guillaumemichel
Copy link
Contributor

@guillaumemichel guillaumemichel commented Apr 9, 2025

Reprovides are currently spiky since all keys are reprovided at once (for both normal and accelerated DHT clients). If new keys are added during a reprovide, the reprovider will wait until the current reprovide is finished before advertising the newly added keys to the DHT. So new keys are blocked by reprovides.

This PR splits the (re)provide queue in a provide queue and a reprovide queue, allowing provides to be advertised immediately, even during reprovides.

ipfs/kubo#10777

Depending on either:

@guillaumemichel guillaumemichel requested a review from a team as a code owner April 9, 2025 08:57
@guillaumemichel guillaumemichel marked this pull request as draft April 9, 2025 13:33
@guillaumemichel guillaumemichel marked this pull request as ready for review April 10, 2025 08:46
@guillaumemichel guillaumemichel changed the title provider: prioritize new provides provider: dedicated provide queue Apr 10, 2025
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Provide collects batches of CID before providing them. This means there may be some delay when filling the batch, especially if each new CID arrives just before the pause detection timer expired. It seems like it might be better to send new provides immediately.

One problem with doing that is that the dublicate CIDs would not be removed, because the batch is currently a map of CIDs. Consider keeping the map to ignore duplicates, but publishing each unique CID immediately. There may be some additional logic needed to keep the map from growing too large, which may mean making it an LRU cache.

Also, we may need a test to show that providing does not need to wait for reprovide.

Otherwise, this all looks good.

@guillaumemichel
Copy link
Contributor Author

Added instant provides but I didn't add tests yet.

I have set instant provides as default with a "safe (?)" default of 128 concurrent provides, batched provides can still be used if the right option is provided. I have set a warning if the queue of cids to be provided instantly isn't processed fast enough. I have set instant provides as the default because before #641, all cids were instantly provided, so the behavior is expected to be similar to what it used to be.

It adds quite some complexity :(

In any case, the provide operation is resource intensive, each provide triggering sending a message to 20 remote peers. With batching and the accelerated DHT client, we can group the messages we send to a remote peer to avoid opening a closing a connection many times to the same peer. But it comes at the expense of timing.

Instant provides are "instant" but the node is expected to open and close more connections, even though the same payload is sent to the same peers.

Some open questions:

  1. Should we get rid of the batched providing altogether? It may seem great for nodes with limited networking capabilities, but it requires to run the accelerated DHT client (which is resource intensive), to be efficient?
  2. If we keep batched providing, should we add a configurable number of workers so that the new provides don't have to wait for the previous batch provide to be finished before being provided?
  3. I think it is a safe behavior to bound the default number of provide workers, but it is hard to come with the right number. I expect accelerated DHT client provides to take ~1s, which means that the number of workers is approx the number of cids/second that the system can provide.

@gammazero let me know if you see anything that can be simplified further

@gammazero
Copy link
Contributor

gammazero commented Apr 11, 2025

  1. Should we get rid of the batched providing altogether? It may seem great for nodes with limited networking capabilities, but it requires to run the accelerated DHT client (which is resource intensive), to be efficient?

If using a worker pool, I do not see a need for batched provides. The size of the worker pool will limit the number of CIDs read into memory and processed.

For lower-priority reprovides, batching could be used to limit the number of CIDs in memory, but this limitation can also be done using a worker pool. It would probably be good to have a smaller worker pool for reprovides than is used for provides. Maybe even limit the reprovide pool size to one or two.

Batching used to be the way CIDs were deduplicated. This has been replaced by the deduplication cache in this PR, so there really does not appear to be any utility in batching. Once #901 is fixed, there will also be no need for deduplication cache.

  1. If we keep batched providing, should we add a configurable number of workers so that the new provides don't have to wait for the previous batch provide to be finished before being provided?

If we have a limited size worker pool, there does not seem to be a need for a batch, since the size of the pool limits the number of CIDs that get loaded into memory for (re)providing. The pool size should be adjustable, and a smaller pool should be used for reprovides by default.

  1. I think it is a safe behavior to bound the default number of provide workers, but it is hard to come with the right number. I expect accelerated DHT client provides to take ~1s, which means that the number of workers is approx the number of cids/second that the system can provide.

If provide sends each CID to 20 remote peers (requires 20 connections) and the worker pool has 50 workers, then that is using 1000 connections. Perhaps the pool size can be informed by the maximum (or a reasonable) number of peer connections, leaving enough capacity to handle incoming peer connections. Does resource manager give us some clue?

@guillaumemichel
Copy link
Contributor Author

The main benefit of batching, is that it reduces the number of connections we need to open. Basically it will

  1. select all the keys from the batch that should be allocated to a DhtServer
  2. open the connection to DhtServer
  3. send all provides to DhtServer (PUT RPC)
  4. close the connection to DhtServer
  5. Repeat with the next peer

Where as if there is no batching, the provider will open a connection to the 20 closest nodes to a cid, send them the PUT request, and continue with the next cid. As a result the node will dial the same nodes many times in a short while if the amount of provided cids is large enough.

Hence batching helps reduce the number of dials. Note that batch providing only works the the accelerated DHT client.


Perhaps the pool size can be informed by the maximum (or a reasonable) number of peer connections, leaving enough capacity to handle incoming peer connections

This is tricky, because the provider has no visibility on the number of (concurrent) connections opened by the DHT. It is true that for each provide the libp2p node will try to open at least 20 connections. However, the normal DHT client limits the number of concurrent connection per request to 10, and the accelerated DHT client has 20 workers. Both default parameters can be overwritten by implementations.

The libp2p resource manager will probably prevent us from opening too many connections (if enabled).

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

See comments. I think we can get rid of batched provides.

@gammazero
Copy link
Contributor

Where as if there is no batching ...
OK, so a little more than simple batching to make use of connections more efficient.

@guillaumemichel
Copy link
Contributor Author

As observed in #901, CIDs are added multiple times to the queue. Deduplication mechanisms were removed in this PR. So each CID may be advertised up to 3 times.

A quick fix is #910, but on the long run we don't need it if we can solve #901.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

LGTM

@guillaumemichel guillaumemichel merged commit 95aa3f0 into main Apr 24, 2025
19 of 20 checks passed
@guillaumemichel guillaumemichel deleted the prioritize-new-provides branch April 24, 2025 07:50
@lidel lidel mentioned this pull request Apr 24, 2025
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants