Cap dirty-block publishing on the gameplay thread

[leroysquad]

Summary

• Only DirtyBlocks is capped. ModifiedBlocks and ModifiedBlocksNoRelight drain fully in the same pass, so packets 47 and 63 still go out before packet 48, and OnNeighbourBlockChange is not deferred past a chunk unload.
• The cap is 512 distinct positions per pass, and it stays on when BlockTicks is turned off. A repeat of a position already taken in that pass does not consume a slot. When the queue is deeper than 8 times the cap, that pass may publish up to 4 times the cap so a deep backlog can shrink.
• The performance report records each queue before and after the pass, plus the peak dirty depth.

The older 0.3 percent and 0.18 TPS figures came from a build that also capped the modified queues. They do not describe this head, so they are not used as evidence here.

Test plan

• One pass of 600 distinct dirty positions leaves 88, and the next pass leaves 0
• 600 copies of one position drain in a single pass
• 600 ModifiedBlocks and 600 ModifiedBlocksNoRelight both drain in one pass
• The same dirty cap holds when BlockTicks is off
• Two-client check that packet 58 arrives for the watcher and not for the excepted client

[leroysquad]

Same-machine /probe profiler 60, send queue on, profile 3. Large world 1-minute TPS went from 0.37 and 0.66 to 1.22 (965 players) and 1.46 (1000 players), about 130k chunks. HandleDirtyAndUpdatedBlocks went from about 35-38 percent of main-thread samples to 0.3 percent. The 512 cap left about 415,000 dirty blocks queued on the first large capture, so those updates wait for later passes. Small world was 17.3 TPS at 623 players versus 24.1 TPS at 447 players, and that 512-chunk world was already at 0 percent in the dirty-block frame.

[leroysquad]

Follow-up on the same machine. The 512 cap kept large-world 1-minute TPS at 1.22 and 1.46, and HandleDirtyAndUpdatedBlocks at 0.3 percent, but the debug log still had about 415,000 dirty blocks queued.

I tried 8192 per pass. On a 977-player, 126,104-chunk capture that dropped 1-minute TPS to 0.18 (mean tick 5.2s) and the queue was still growing, from about 22,000 to 57,000 during the probe minute. The default is back at 512. The debug line is now once a second instead of every pass.

The backlog is the remaining problem. Each publish is still one exchange-block packet per watching client, so a bigger cap just spends the tick on that again.

[Pixnop]

Requesting changes. The idea is sound, and the dirty-block half is safe as written. The problem is the two modified-block queues.

Capping ModifiedBlocks lets block entity packets overtake their block. UpdateEvery100ms still calls SendDirtyBlockEntities, uncapped, right after the capped drain. Vanilla always sent the block packet (47 or 63) for every queued position before the block entity packet (48) in the same pass. With more than 512 modified positions, the block entity for position 513 and later now goes out one or more passes before its block. The client creates that block entity on top of the old block, and when the block packet arrives later the bulk commit replaces it with a fresh, empty one. No second packet 48 follows. Paste a schematic of more than 512 blocks holding signs, chiseled blocks or typed chests next to a second player: past the 512th position they render blank until a relog. The ordering is from the code this PR generates; the client outcome is from reading the client code, not from a live run.

The capped drain also defers OnNeighbourBlockChange, and can lose it. The modified drains do not only publish, they run the self update for each position. If the chunk unloads before its entry is drained, RelaxedBlockAccess.GetBlock returns air and the callback silently does nothing. Worldgen scheduled updates are enqueued exactly once and then cleared from the saved chunk, so a rivulet source generated under backlog never starts flowing. That is a saved difference from vanilla, not a delay.

The simplest shape that keeps the win: cap DirtyBlocks only. There the block id is read at drain time, so the cap is a pure delay, and the per-chunk grouping is what saves the per-client walk. Let the two modified queues drain fully, or cap them only together with the block entity and decor sends, so the order holds.

The rest:

• Drain rate. The cap is per pass, and passes run at most once per server tick, halved by the adaptive throttle once a tick exceeds 60 ms. With your large-world numbers (ticks of 671 to 829 ms) that is 620 to 760 entries a second. The 415,000 backlog needs about ten minutes with no new input, and you saw it growing. The budget is charged per dequeued entry, so duplicates and positions no client has loaded cost as much as real ones. A time budget per pass, or a cap that scales with the backlog, would degrade more gracefully. Also, turning BlockTicks.Enabled off while a backlog exists publishes all of it in one tick.
• The default. The comment says 8192 dropped TPS to 0.18, so 512 stays. Your own table puts the dirty frame at 0.3 percent of the tick at 512, about 2.5 ms. Sixteen times that is 40 ms, not a five-second tick. The 8192 run applied the same budget to both modified queues, each running up to 8192 neighbour updates and broadcasting to every client. A per-helper profile at 512 and at 8192, plus one run with the grouping alone, would show which queue cost the time.
• Stats. The report only echoes the configured cap. What an admin needs, queued before and after per queue, exists only as a debug log line.
• Tests. Nothing under tests/ changes and no scenario touches these queues, so the 49 pass identically with the change reverted. A scenario fits the harness: mark 600 positions dirty in a chunk an observer has loaded, assert 88 are still queued after one pass and none after the next, and add the usual Disabled twin class. Assert on the queue sizes; Atlas's player.Client does not decode the exchange packet, so reading packet 58 would need the raw probe from VanishPrivacyScenarios.

Hygiene is clean: 22 hunks consistent, the patch applies to a pristine baseline, markers are in place, both builds are green, 49/49.

[Zaldaryon]

Requesting changes. The dirty-block cap is separately measured, but the two modified-block queues have correctness and recovery gaps.

The capped drains publish block packets 47/63, while SendDirtyBlockEntities remains uncapped and runs immediately after the drain. With more than 512 modified positions, packet 48 can arrive for a later position before that position's block packet. The later block packet can replace the block entity, leaving signs, chiseled blocks, or typed chests blank until a relog. Coordinate the modified queues with dependent block-entity/decor sends, or keep those queues full-drain, and test more than 512 positions across multiple passes with a second client.

The capped drain also consumes deferred OnNeighbourBlockChange work. If the chunk unloads before the entry is processed, the callback can be lost; worldgen scheduled updates have already been cleared from saved chunk state. Preserve the callback across unload or prove the queue is persisted, and add a regression for deferred callbacks and config disablement with backlog.

The PR's ordinary-client propagation test is still unchecked, and GitHub reports no checks for this head. The measured backlog is about 415,000 entries while the cap does not keep up; report per-queue backlog/throughput and verify that disabling the feature cannot turn the backlog into a single-tick drain.

[Zaldaryon]

Since my review, #347 merged and advanced indev from ece5d42 to c5c3aee. Please rebase onto current indev before the next review and rerun the required validation on the updated head.

[leroysquad]

7ed3995 caps DirtyBlocks only. The default stays 512. ModifiedBlocks and ModifiedBlocksNoRelight use the vanilla full drain, so packets 47 and 63 still go out before packet 48 in the same pass, and OnNeighbourBlockChange is not left queued across a chunk unload. The dirty cap also runs when BlockTicks.Enabled is false, so turning that switch off does not publish a dirty backlog in one tick. The performance report records dirty, modified, and no-relight counts before and after the pass.

DirtyBlockBudgetScenarios marks 600 positions and expects one pass to drain 512. The disabled twin uses the same check with block ticks off. Those scenarios were not executed in this pass, and there is no second-client schematic run.

On the captures already posted: 512 held 1-minute TPS at 1.22 (965 players) and 1.46 (1000 players), about 130k chunks, with HandleDirtyAndUpdatedBlocks at 0.3 percent. 8192 on a 977-player, 126,104-chunk capture dropped 1-minute TPS to 0.18. The cap was not raised. indev is at c5c3aee. This branch was not rebased.

[leroysquad]

Rebased onto current indev (c5c3aee). New head: a26d62f.

[Zaldaryon]

The DirtyBlockBudget scenarios pass on Windows and Linux. The current patch caps only DirtyBlocks and applies that budget regardless of BlockTicks; the PR description and measurements still describe an earlier version that capped all three queues only while block ticks were enabled. Please update the body to match this head and provide final-code performance evidence plus a normal two-client block-propagation run across multiple deferred passes, including visibility for the second client and unaffected clients.

Local validation on head a26d62fcd9810d2690887038b342f6284c2a20ae: Windows and Linux bootstraps with ILSpy 10.1.0.8386 passed; both Release builds had 0 errors; both smoke runs reached WorldReady; DirtyBlockBudget passed 2/2 on each OS. GitHub has no build or test checks.

PR #344 has since merged into indev at 66a8530718e254a5b055908140dcff7c425bfd16. This head still records base c5c3aee. Please rebase onto current indev and rerun the build and scenario gates with the requested client and performance evidence before approval.

[Pixnop]

Still requesting changes, but the two correctness problems from my last review are gone. Capping only DirtyBlocks is the right shape. The ModifiedBlocks and ModifiedBlocksNoRelight drains are back to vanilla line for line, so packets 47 and 63 go out before packet 48 again, and OnNeighbourBlockChange no longer outlives its pass. The dirty cap is a pure delay: the block id is read when the entry is published, and the per-chunk loop keeps the vanilla filters (connected player, DidSendChunk, except client). The cap no longer depends on BlockTicks.Enabled, so turning that switch off cannot flush a backlog in one tick, and the report now shows each queue before and after the pass.

On a26d62f I ran bootstrap, a Release build (0 errors, no new warnings in the touched files) and the full suite: 51/51. The two new scenarios passed 2/2 five times in a row. They catch both earlier mistakes: removing the cap turns both red, capping ModifiedBlocks at 512 again turns both red, and making the cap depend on BlockTicks.Enabled turns the Disabled twin red. The patch applies to a pristine baseline and gives the generated file.

What still blocks:

1. The drain still falls behind under the load it was written for. StratumDrainDirtyBlocks (patch line 905) takes a fixed 512 entries per pass, and each dequeued entry spends a slot before anything else is checked, so repeat positions and positions in chunks no client has loaded cost as much as real updates. UpdateEvery100ms runs at most once per server tick. At your large-world ticks of 671 to 829 ms that is about 620 to 760 entries a second. Your run ended with about 415,000 queued, so a packet 58 behind that backlog waits about ten minutes even with no new input, and whenever input outruns that rate the queue only grows. Vanilla sends the same entry on the next pass. At normal TPS the cap drains about 5,120 entries a second, so ordinary servers will rarely hit this, but the large world is the case this PR is for. Collapsing repeat positions within a pass loses nothing, since the id is read at publish time, and a time budget per pass or a cap that scales with DirtyBlocks.Count would let the queue converge. Those captures ran on e34d88b, whose dirty drain is identical to this head, but the head's tick time has not been measured (see 3).

2. Nothing checks what clients receive, and two of the asserts cannot fail. MeasureOnGameThread returns the constant Queued - Cap (DirtyBlockBudgetScenarios.cs:98) instead of the dirtyAfter it measures at line 80, so Assert.Equal(Queued - Cap, dirtyLeft) at line 29 and Assert.Equal(88, dirtyAfter) in DirtyBlockBudgetDisabledScenarios.cs:23 compare a constant with itself. The real check is the throw at line 82. Nothing reads the dirty queue after the capped pass, so nothing shows the 88 leftovers go out on the next one. ModifiedBlocksNoRelight is never filled: putting the 512 cap back on it (the packet 63 path from my last review) leaves both scenarios green, and so does any ModifiedBlocks cap from 513 to 599, since the check is modifiedAfter < 88 on 600 entries. Removing the RecordDirtyBlockPublish call keeps them green too, because the report check only looks for the Dirty publish: label. The send loop has no coverage at all: deleting the server.SendSetBlock(client.Player, ...) call (patch line 941), or dropping the except-client skip at line 937, leaves the full suite at 51/51. I suggested queue sizes last time, and that is the right check for the cap, but this loop replaces vanilla's broadcast and the test-plan box for a normal client is still unchecked. Please return the measured count, run one more pass and assert the dirty queue is empty, fill both modified queues with more entries than any plausible cap and assert both drain in one pass, and either extend PacketProbe from VanishPrivacyScenarios to decode packet 58 for two players who have the chunk (the excepted one gets none of the positions, the other gets all of them with the current id across the 512 and 88 passes) or post the two-client run Zaldaryon asked for.

3. The figures in the config comment and the PR body describe the earlier version. StratumConfig.cs:1531-1533 justifies 512 with "0.3 percent of the tick" and "8192 ... dropped 1-minute TPS to 0.18". Both come from e34d88b, ff28f13 and 855bccf, which also capped both modified queues. On this head those queues drain fully every pass, so neither the share of HandleDirtyAndUpdatedBlocks at 1000 players nor the cost of 8192 dirty entries alone has been measured. The body still says all three queues are capped only while block ticks are enabled and that turning the switch off keeps the vanilla drain; both are false here, and the table comes from the same captures. Please run /probe profiler 60 on this head at 512, at 8192 and once with the grouping but no cap, at the same player and chunk counts, or drop the numbers from the comment, and update the body.

Notes, not blocking:

• The committed patch is not what scripts/extract-patches.sh produces. Hunk @@ -667,41 +958,48 @@ deletes and re-adds both modified drains unchanged (27 lines each side, identical to the baseline). Regenerating from the same generated file gives @@ -667,15 +958,20 @@, and the index line says d1f5a50 where the result hashes to 7978b83. The code is the same, but the diff makes the modified queues look touched. Re-running the extraction fixes it.
• BlockTicks.Enabled=false no longer restores the vanilla dirty drain. This is now the only BlockTicks budget that ignores Enabled. The comment and the Disabled scenario make that intentional, which is fine, but the only way back to vanilla is a very large MaxDirtyBlocksPerPass.
• The Dirty publish line covers the last pass only, with no peak and no rate, so whether the queue converges is still not visible. A peak backlog or entries per second would answer Zaldaryon's throughput point.
• The head is one commit behind indev (#344, which only touches ServerSystemEntitySimulation.cs.patch). git merge-tree is clean, so a maintainer can run /rebase.

[leroysquad]

Pushed 456bb0c.

• Repeat positions in one pass do not spend a cap slot. The id is still the latest one read at publish time.
• The base cap stays 512. When the queue is deeper than 8 times that, the pass may publish up to 4 times the cap. A 600-entry run still stops at 512.
• The scenarios return the measured dirty count, run a second pass and expect an empty dirty queue, and fill both modified queues with 600 entries and expect both to drain in one pass. 600 copies of one position must drain in one pass.
• The config comment no longer cites the 0.18 TPS figure. That run also capped the modified queues. This head does not.

I have not run the scenario suite or a new profiler capture on this commit. The two-client packet 58 check is still open.

[leroysquad]

Ran the dirty-block scenarios on 456bb0c after a clean bootstrap with ilspycmd 10.1.0.8386. Release build had 0 errors. scripts/scenarios.ps1 --filter DirtyBlock passed 3/3 in 9 seconds: the 600-distinct pass leaves 88 then 0, 600 copies of one position drain in one pass, and both modified queues of 600 drain in one pass with BlockTicks off as well. The two-client packet 58 check is still not done.

[leroysquad]

@Pixnop @Zaldaryon 456bb0c is up for another look. Repeat positions do not spend a cap slot, a deep queue may publish up to 4x the 512 cap, and the scenarios read the real queues. On this machine --filter DirtyBlock passed 3/3. The two-client packet 58 check is still open.

[Zaldaryon]

The changes in 456bb0c address key review concerns: duplicate positions in one pass no longer spend cap slots, deep queue scaling is bounded up to 4x the base cap, and the scenarios now measure real queue drain counts (dirtySecond == 0, modifiedAfter == 0, noRelightAfter == 0).

What still blocks approval:

1. The branch is based on c5c3aee. Current indev is at a3966e2 (#344, #352, #353 merged). On Windows with ilspycmd 10.1.0.8386, scripts/bootstrap.ps1 against c5c3aee fails to apply 12 other patches. On a3966e2, bootstrap.ps1 and the Release build pass with 0 errors. Please rebase onto current indev.
2. The normal two-client block propagation check across deferred passes remains open. As noted in the comments, the two-client packet 58 check has not been run. Verifying that viewing clients receive deferred updates while excepted clients are omitted is required before landing this broadcast path change.
3. Final-code profiler and performance evidence on this exact head is still needed. The PR body still cites figures from earlier runs that capped modified queues as well. Please update the description and checklist with measurements from the final implementation.

Non-blocking:

• In ServerSystemBlockSimulation.cs.patch, re-running scripts/extract-patches.ps1 will clean up the unchanged modified-queue context lines.

[Pixnop]

Still requesting changes, but 456bb0c answers most of my last review. Repeat positions in a pass no longer spend a slot, and since the block id is still read for every dequeued entry, the merged update carries the latest one. The scenarios now return the measured count, a second pass has to leave the dirty queue empty, both modified queues get 600 entries and must drain in one pass, and the report checks modified=600->0 and noRelight=600->0. The config comment and the body no longer quote figures from the three-queue builds, and the report has a peak dirty depth. The modified drains are still vanilla line for line, so packets 47 and 63 still go out before 48 and OnNeighbourBlockChange still runs in its own pass.

On 456bb0c I ran bootstrap (195 patches, 0 failed), a Release build (0 errors, no new warnings in the touched files) and the full suite: 52/52. The three DirtyBlock scenarios passed 3/3 seven times, under fr_FR, C.UTF-8 and invariant globalization. The holes from last time are closed: capping either modified queue at 512, or ModifiedBlocks at 599, turns both main scenarios red, and so do removing RecordDirtyBlockPublish and a leftover that never drains. Making repeats spend a slot turns RepeatedPosition_Should_NotConsumeTheCap red with 88 left.

What still blocks:

1. A large MaxDirtyBlocksPerPass now stops dirty publishing entirely. The deep-backlog rule (patch lines 518-520) computes stratumDirtyBudget * 8 and stratumDirtyBudget * 4 in int. VintagestoryLib builds unchecked (CheckForOverflowUnderflow is False in VintagestoryLib.csproj:10), and EnsureSane (StratumConfig.cs:1555) only clamps the lower bound. With 2147483647, * 8 wraps to -8, so the branch runs on any queue, and * 4 wraps to -4, so the budget is -4 and while (taken < budget ...) at line 915 never dequeues. This drain is the only consumer of DirtyBlocks, so no packet 58 from MarkBlockDirty reaches any client again and the queue grows until restart. I checked it with a throwaway fixture: at 2147483647 and at 1000000000, 600 queued entries were still 600 after 1 pass and after 11 passes, while 100000 drained them in one. By arithmetic 536870912 and 1073741824 also end at zero or below, and 1073741825 publishes 4 per pass. On a26d62f the cap went straight to the drain, so int.MaxValue was the vanilla drain, and my last review named a very large cap as the only way back to vanilla. Only an operator can set it, but that is exactly the value an operator would pick. No scenario reaches the branch: removing the whole 8x/4x rule leaves the suite at 52/52. Please do the arithmetic in long (if (waiting > budget * 8L) budget = (int)Math.Min(waiting, budget * 4L);) or clamp the value in EnsureSane, and add two fixture cases: a small cap that triggers the rule (for example 16 with 200 distinct queued, expecting 64 drained) and int.MaxValue, expecting the queue empty after one pass.

2. The deep-backlog rule has not run on the world it is for. Your large-world queue reached about 415,000, far past 8 x 512, so at that depth every pass publishes up to 2048 distinct positions, a budget nobody has profiled. The patch comment ("A 400k queue can shrink instead of only growing") and the config comment ("so a deep backlog can shrink") state the result as fact. With no new input the rule does what it says: on this head a 100,000-entry queue empties in 55 passes (47 at 2048, 7 at 512, one of 160), where a fixed 512 needs 196. With input, the numbers we have settle nothing. At about 2,150 entries a second (your 8192 run, 22,000 to 57,000 in one minute), 2048 per pass converges at your measured 0.68 to 0.82 s ticks, slowly, and only while the tick stays under about 0.95 s. If input scales per tick instead, it converges only if repeats outnumber distinct positions about five to one, and nobody has measured that ratio. Entries in chunks no client has loaded still spend a slot. A pass never does more work than vanilla's full drain of the same queue, so this is not a TPS regression against indev, but convergence is the claim the PR rests on. Please run /probe profiler 60 on this head on the large world with default settings and post the 1-minute TPS, the share of HandleDirtyAndUpdatedBlocks, and the dirty depth at the start and end of the minute. If that capture is not possible, drop the shrink wording from both comments. This is also the final-code measurement Zaldaryon is waiting on.

3. The send loop still has no test, and the merge changes who gets a packet. All three scenarios assert queue counts and report strings, and the entries are dequeued before the send loop runs. Deleting server.SendSetBlock(client.Player, ...) at patch line 963, or the except-client skip at 959-962, leaves the full suite at 52/52, and sending block id 0 for every update leaves the DirtyBlock scenarios at 3/3. Every scenario enqueues W=0, so the except path never runs. I wrote a throwaway probe that decodes packet 58 from the dummy connections, to check the loop by hand. With 600 distinct positions all excepting the actor, the watcher and a third player got 512 after the first pass and 600 after the second, every one with the current id. The actor got none, and a player 3000 blocks away got none. So the loop is right for distinct positions, and the probe goes red under each of the three mutations above. It also found one difference from vanilla. When a position repeats in a pass, line 925 (prior.ExceptClientId = result.W;) keeps only the last entry's except id. With 4 positions each queued twice, first excepting the actor and then excepting the watcher, this head sent the watcher nothing, where vanilla sent it all 4 through the first entries. In vanilla content only BEChisel passes a skip player, and its packet 48 covers the gap, but a mod calling MarkBlockDirty(pos, player) relies on that 58. Please keep the exclusion only when both entries agree (prior.ExceptClientId = prior.ExceptClientId == result.W ? result.W : -1;) and commit a packet 58 check: extend PacketProbe from VanishPrivacyScenarios to decode packet 58 for two players who have the chunk, enqueue with one player's client id as W, assert the other sees all 600 positions with the current id across both passes and the excepted one sees none, and add one position queued with two different except ids. That also closes the last test-plan box, which Zaldaryon is blocking on.

4. The head no longer merges into indev. It is 3 commits behind (#344, #352, #353), GitHub reports it as conflicting, and git merge-tree against a3966e2 stops on ServerSystemBlockSimulation.cs.patch with 20 conflict regions. Most are the index line and hunk headers, but #353 removed the position pool (StratumPosPoolSize, stratumPosPool, stratumFluidPosPool) from the same field block where this PR adds its dirty fields, so a maintainer cannot use /rebase this time. Watch the shortcut: taking this head's whole patch file applies cleanly and brings back the shared position pool #353 removed. Please bootstrap current indev, reapply only the dirty-drain change to the generated file, run scripts/extract-patches.sh, and check that no stratumPosPool or StratumGetPooledPos is left. That also settles the extraction note from my last review, which is still open and slightly worse: hunk @@ -667,41 +958,56 @@ still deletes and re-adds both modified drains unchanged, the index line still says d1f5a50 where the result hashes to 80aa113, and 19 of the 22 hunk headers have a new-start off by 1 or 9. To see whether the code survives the move, I ported it onto a3966e2 by hand: build 0 errors, DirtyBlock 3/3, full suite 56/56, and the regenerated patch has two clean hunks there (@@ -667,15 +954,28 @@ and @@ -699,10 +999,12 @@).

Note, not blocking:

• Repeats never count now, so a pass keeps dequeuing until it has budget distinct positions or the queue is empty. That is never more work than vanilla, but the summary on DirtyBlockBudgetDisabledScenarios ("must not turn a dirty backlog into a single-tick drain") now reads broader than the code, since a queue of repeats does drain in one tick. Is that intended? If so, saying that publishing is capped would match what it tests.