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.