perf(light): resolve the opposite face once instead of per neighbour - #28
Merged
Conversation
BlockFace#opposite() is a switch over the enum, and the inner loop of a propagation calls it once per face per queued position - six times for every block the light reaches. Resolving all six at class load turns that into an array read. baseline 32.64 / 32.47 us optimised 27.15 / 28.45 us -4.75 us, -14.6 % AMD Ryzen 7 5800X, JDK 25.0.3, 3 forks, 3 warmup and 5 measurement iterations of 1 s, single thread, LightEngineStageBenchmark.falcoPropagate. The measurement needed two attempts, and the first one would have been wrong. A single before-and-after gave -15.6 % with separated intervals, which looked conclusive; re-measuring the baseline afterwards gave 46.11 +- 15.14 us, 12.80 us away from the first baseline and further than the effect being claimed. The machine was under load average 8.5 from leftover daemons. The figures above are from an interleaved base/opt/base/opt run on an idle machine, where the two baselines land 0.17 us apart - the effect is now four times the repeat spread rather than a third of it. propagate was the dominant stage of the light path at 31.41 us in Measured Results, which is what put it ahead of the opacity tables at 8.07. ChunkLightPropagator has the same call in its own inner loop and is deliberately not touched here: it walks a whole chunk rather than a section and deserves its own before-and-after rather than being changed on the strength of this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Test results 195 files 195 suites 3m 42s ⏱️ Results for commit 6c32393. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
After #26 moved the opacity tables off the top spot,
propagatewas the dominant stage of the lightpath at 31.41 µs against 8.07 for the tables. This takes 14.6 % off it.
BlockFace#opposite()is a switch over the enum, and the inner loop calls it once per face perqueued position — six times for every block the light reaches. Resolving all six at class load
turns it into an array read.
AMD Ryzen 7 5800X, JDK 25.0.3, 3 forks, 3 warmup and 5 measurement iterations of 1 s, single
thread,
LightEngineStageBenchmark.falcoPropagate.The measurement needed two attempts, and the first would have been wrong
A single before-and-after gave −15.6 % with separated confidence intervals — by the usual test,
conclusive. Re-measuring the baseline afterwards gave 46.11 ± 15.14 µs: 12.80 µs away from the
first baseline, and further than the effect being claimed. The machine was at load average 8.5
from leftover Gradle daemons, and the intervals had only bounded within-fork variance, not the drift
between runs.
The figures above come from an interleaved
base/opt/base/optrun on an idle machine. The twobaselines land 0.17 µs apart, so the effect is roughly four times the repeat spread instead of a
third of it.
This casts doubt on the numbers in #26, taken during the same busy period: the opacity share was
19.4 / 20.4 / 15.7 % across three forks, and a single-fork run of the same benchmark had given
21.4 / 19.4 / 18.8 %. The conclusion there — a sixth to a fifth, not the largest block — survives the
doubt, since it rests on a ratio measured within each run rather than on absolute times. The exact
percentages deserve a re-run on an idle machine before anyone quotes them. Say the word and I will
redo them and correct the wiki table.
What is deliberately not in here
ChunkLightPropagatorhas the same call in its own inner loop. It walks a whole chunk rather than asection, so its before-and-after is a different measurement, and changing it on the strength of this
one would be exactly the shortcut the paragraph above argues against.
Types of changes
Checklist
test<What><Expectation>, and use plain JUnit assertions@NotNull; the package@NotNullByDefaultcovers it./gradlew buildis green locally — 673 tests, 0 failuresNo new test: the behaviour is unchanged, and that is the claim the existing suite already checks —
including the equivalence tests against Minestom's engine and the byte-identity checks, which is what
makes an array lookup instead of a switch safe to assert.
Documentation: the stage table in Measured Results carries 31.41 µs for
propagateand should getthis figure, but see the doubt above — I would re-run both this and the #26 numbers together on an
idle machine and correct them in one pass rather than adding one number to a table whose neighbours
are suspect.
Measurements
thread count (
-t)Single-threaded, which is what the stage benchmark measures; the concurrency of a pass lives one
level up in the scheduler.
🤖 Generated with Claude Code