Delay updateAll() & only update subset immediately#815
Conversation
If the number of peers is greater than or equal to `MIN_PEER_UPDATE_ALL` (currently `10`), only a subset of the peers are scanned immediately. A full scan is schedule later with a delay based on the number of peers. `Math.sqrt(this.peers.length) * 100ms` `updateAll()` now takes an optional `limit` arg so the code is reused between the subset & complete scans. All calls to `updateAll()` were refactor to `queueUpdateAll()` except in `core.truncate()` as that is assumed to be strongly propagated.
| this._notDownloadingTimer = null | ||
|
|
||
| this._updateAllBump = null | ||
| this._updateAllBound = () => { |
There was a problem hiding this comment.
use a bind instead on an instance method
There was a problem hiding this comment.
I needed to set _updateAllBump to null so it would reschedule. would normally put in updateAll but wanted to reuse it for both subset and full scan.
There was a problem hiding this comment.
thats ok, just define this as _something and then bind that here.
There was a problem hiding this comment.
actually much easier if you add to updateAll no?
if (this._updateBump !== null) {
clearTimeout(this._updateBump)
this._updateBump = null
}
then my below comment on the clear is irrelevant and you can rmeove the clearTimeotu down there also from the else
There was a problem hiding this comment.
Adjusted so the bound version is just a .bind() and moved the clear to updateAll(). It checks if a full scan before clearing since the method is used for the subset as well.
|
|
||
| queueUpdateAll() { | ||
| if (this.peers.length >= MIN_PEER_UPDATE_ALL) { | ||
| const MIN_DELAY_UPDATE_ALL = Math.sqrt(this.peers.length) * 100 |
There was a problem hiding this comment.
normal case, its not a constant. I think you should define a function isntead that just makes this static
getDelay(peers) {
if (peers < 10) return 100
if (peers < 50) return 200
if (peers < 100) return 300
if (peers < 400) return 400
return 1000
}
or whatever we want
There was a problem hiding this comment.
Do we still want it to be square root? I like that it doesn't grow linear just in case.
| const LAST_BLOCKS = 256 | ||
|
|
||
| const MAX_RANGES = 64 | ||
| const MIN_PEER_UPDATE_ALL = 10 |
There was a problem hiding this comment.
we should prob set a bit higher, like
| if (this._updateAllBump !== null) return //skip if already scheduled | ||
| this._updateAllBump = setTimeout(this._updateAllBound, this.getUpdateAllDelay(this.peers)) | ||
| } else { | ||
| clearTimeout(this._updateAllBump) |
There was a problem hiding this comment.
need to set bump to null post this btw, otherwise live lock per above
There was a problem hiding this comment.
There was a problem hiding this comment.
mmm, you are cancelling that call here tho
There was a problem hiding this comment.
ah, you're right sorry. i have fixed per the other comment.
|
|
||
| getUpdateAllDelay(peers) { | ||
| return Math.sqrt(peers.length) * 100 | ||
| return Math.min(3000, Math.max(100, (peers.length - MIN_PEER_UPDATE_ALL) * 5)) |
There was a problem hiding this comment.
ok if MIN_PEER_UPDATE is relatively non small, like 40-50
There was a problem hiding this comment.
Updated MIN_PEER_UPDATE_ALL to 40. Previously was 30.
|
Merged |
|
I got curious about this PR, |
|
The regression I am analysing: test('delayed updateAll clears timer after reorg skip', async function (t) {
const Replicator = require('../lib/replicator')
const r = new Replicator({})
t.teardown(() => {
if (r._updateAllBump) clearTimeout(r._updateAllBump)
})
r.peers = Array.from({ length: 40 }, () => ({}))
r.getUpdateAllDelay = () => 1
// Simulate the delayed full scan firing while a reorg is active
r._applyingReorg = {}
r.queueUpdateAll()
await new Promise(resolve => setTimeout(resolve, 10))
t.is(r._updateAllBump, null, 'timer handle should be cleared after firing')
}) |
Update all is too aggressive at the moment and tries to bump any pending ranges etc every time. This is meant to cover edgecases but that can be done via a delay instead.
If the number of peers is greater than or equal to
MIN_PEER_UPDATE_ALL(currently10), only a subset of the peers are scanned immediately. A full scan is schedule later with a delay based on the number of peers.Math.sqrt(this.peers.length) * 100msupdateAll()now takes an optionallimitarg so the code is reused between the subset & complete scans.All calls to
updateAll()were refactor toqueueUpdateAll()except incore.truncate()as that is assumed to be strongly propagated.