-
Notifications
You must be signed in to change notification settings - Fork 209
Fix idle range completion scan #832
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
f9700d5
1414e16
76376ac
60823b1
5403eb5
37224a9
ffd7033
fe789a1
62096ed
aded7ec
11e0082
0e24e5a
51738b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2600,23 +2600,60 @@ module.exports = class Replicator { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| _updateRanges(index, limit) { | ||||||||||
| if (this._ranges.length === 0) { | ||||||||||
| return { checked: 0, resolved: 0, index: 0 } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| index = Math.min(index, this._ranges.length - 1) | ||||||||||
|
|
||||||||||
| let checked = 0 | ||||||||||
| let resolved = 0 | ||||||||||
| let remaining = Math.min(limit, this._ranges.length) | ||||||||||
|
|
||||||||||
| while (remaining-- > 0 && this._ranges.length > 0) { | ||||||||||
| if (index >= this._ranges.length) index = 0 | ||||||||||
|
|
||||||||||
| const r = this._ranges[index] | ||||||||||
|
|
||||||||||
| clampRange(this.core, r) | ||||||||||
| checked++ | ||||||||||
|
|
||||||||||
| if (r.end !== -1 && r.start >= r.end) { | ||||||||||
| this._resolveRangeRequest(r) | ||||||||||
| resolved++ | ||||||||||
| } else { | ||||||||||
| index++ | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| index = this._ranges.length === 0 ? 0 : index % this._ranges.length | ||||||||||
|
|
||||||||||
| return { checked, resolved, index } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // "slow" updates here - async but not allowed to ever throw | ||||||||||
| async _updateNonPrimary(updateAll) { | ||||||||||
| // Check if running, if so skip it and the running one will issue another update for us (debounce) | ||||||||||
| while (++this._updatesPending === 1) { | ||||||||||
| let len = Math.min(MAX_RANGES, this._ranges.length) | ||||||||||
| let checkedSinceResolve = 0 | ||||||||||
| let rangeIndex = 0 | ||||||||||
| const drain = this._inflight.idle || updateAll | ||||||||||
|
|
||||||||||
| for (let i = 0; i < len; i++) { | ||||||||||
| const r = this._ranges[i] | ||||||||||
| while (this._ranges.length > 0) { | ||||||||||
| const limit = Math.min(MAX_RANGES, this._ranges.length - checkedSinceResolve) | ||||||||||
| const result = this._updateRanges(rangeIndex, limit) | ||||||||||
| const { checked, resolved } = result | ||||||||||
| rangeIndex = result.index | ||||||||||
|
|
||||||||||
| clampRange(this.core, r) | ||||||||||
| if (resolved > 0) checkedSinceResolve = 0 | ||||||||||
| else checkedSinceResolve += checked | ||||||||||
|
|
||||||||||
| if (r.end !== -1 && r.start >= r.end) { | ||||||||||
| this._resolveRangeRequest(r) | ||||||||||
| i-- | ||||||||||
| if (len > this._ranges.length) len-- | ||||||||||
| if (this._ranges.length === MAX_RANGES) updateAll = true | ||||||||||
| } | ||||||||||
| if (checked === 0 || checkedSinceResolve >= this._ranges.length) break | ||||||||||
| if (!drain) break | ||||||||||
|
|
||||||||||
| await yieldToLoop() | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previous behavior had implied event loop starvation prevention mechanics: Lines 2607 to 2609 in c2ee97e
In this PR I am trying to preserve this behavior by simultaniously also fixing the bug: while (this._ranges.length > 0) {
this._updateRanges(...MAX_RANGES...)
...
await yieldToLoop()
}Also possible that debounce/coalescing mechanisms could be more probable to be invoked in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An additional improvement we can do here is to assert this behavior with a test
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcus-pousette-hp the debouncing/guard via Line 1545 in ddd6c79
Here's a test case for asserting that seeks are processed relatively quickly. Before the PR it passes and after this PR it fails: test.solo('processing ranges doesnt block seeks', async function (t) {
const core = await create(t)
const clone = await create(t, core.key)
// Well above MAX_RANGES so draining the backlog takes many batches: 50k/64
const totalLength = 50_000
await core.append(new Array(totalLength).fill('a'))
// Make all range requests stay in `_ranges` forever so `_updateNonPrimary` has an oversized backlog to scan.
await core.clear(0, totalLength)
replicate(core, clone, t)
// Populate ranges
for (let i = 0; i < totalLength; i++) {
clone.download({ start: i, end: i + 1 }).done().catch(noop)
}
t.is(clone.core.replicator._ranges.length, totalLength, 'large range backlog is pending')
const bytesBefore = core.byteLength
// Appending triggers an upgrade on clone, kicking off `_updateNonPrimary` to attempt the backlog.
// Seeking to `bytesBefore` needs that same upgrade (so clone doesnt know about it).
await core.append('end')
// Baseline (no range backlog) resolves this in ~1ms; 900ms+ with the backlog
const BUDGET = 100
const start = Date.now()
let delta = -1
const seek = clone.seek(bytesBefore).then(() => {
delta = Date.now() - start
})
await once(clone, 'append')
// To allow merkle IO for seek
await new Promise((resolve) => setImmediate(resolve))
t.is(clone.core.replicator._seeks.length, 0, 'seek resolved after upgrade received')
await seek
t.ok(delta <= BUDGET, `seek resolved within ${BUDGET}ms despite a ${totalLength}-range backlog`)
t.comment('seek time', delta)
t.is(clone.core.replicator._ranges.length, totalLength, 'range backlog is still pending')
})Test made with a litte AI assistance. |
||||||||||
| if (this.destroyed) break | ||||||||||
| } | ||||||||||
|
|
||||||||||
| for (let i = 0; i < this._seeks.length; i++) { | ||||||||||
|
|
@@ -3384,6 +3421,10 @@ function incrementRx(stats1, stats2) { | |||||||||
|
|
||||||||||
| function noop() {} | ||||||||||
|
|
||||||||||
| function yieldToLoop() { | ||||||||||
| return new Promise((resolve) => setTimeout(resolve, 0)) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| function backoff(times) { | ||||||||||
| const sleep = times < 2 ? 200 : times < 5 ? 500 : times < 40 ? 1000 : 5000 | ||||||||||
| return new Promise((resolve) => setTimeout(resolve, sleep)) | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the old this._ranges.length === MAX_RANGES special case because it only re-triggered updateAll when the queue shrank to exactly the cap, and I couldn’t find test coverage or PR discussion showing that boundary behavior is intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual MAX_RANGES cap is still preserved. The new cursor loop drains completed ranges in capped chunks without relying on that exact effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this should be removed. The updateAll prompts requesting ranges from a random subset of peers potentially. Since the
_updateNonPrimaryfunction is called only when either receiving data or an upgrade, it could deadlock with requests queue in_rangesthat haven't been requested of peers that can fulfill them ifupdateAll()isn't triggered.The reason for the exact match is that we know in this scenario only 1 range was resolved so this check triggers as soon as there can be at least one range that could not have been requested of peers. Aka triggering another set of range requests could prompt a response which resolves a range not included in the last
_updateNonPrimaryrun.This doesn't prevent from large backlogs not getting stuck (hence the need for this PR) when there is more unfulfilled range request than the MAX_RANGES though. I don't think it should be
updateAll = trueforthis._ranges.length >= MAX_RANGESthough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ye, that make sense. I restored this in 5403eb5, preserving the exact this._ranges.length === MAX_RANGES boundary. I also added a regression test with 65 one-block ranges and two peers as an proof/argumetn for this. Though it is quite verbose right now, I will re-iterate on this tomorrow.