Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the Java Microbenchmark Harness (JMH) to the project, adding a dedicated benchmark source set and several performance tests for bounding volumes, light lists, and geometry lists. In LightList, several methods including remove, clear, sort, and update were optimized using System.arraycopy and Arrays.fill to improve efficiency. Feedback suggests removing the distToOwner array and its associated logic in LightList, as it is currently dead code and not correctly synchronized during element removal.
5906442 to
bd6da59
Compare
riccardobl
left a comment
There was a problem hiding this comment.
Benchmarks should be moved into their own jme3-benchmark module, so that we don't have to deal with them in the normal codebase.
Other than that and the distToOwner comment by gemini, everything looks good.
4400dac to
e2dfcf2
Compare
|
Should be solid now |
e2dfcf2 to
5fe394e
Compare
5fe394e to
f67972b
Compare
|
@riccardobl could you take another look? |
|
@copilot resolve the merge conflicts in this pull request |
f67972b to
1854dd1
Compare
1854dd1 to
b26c971
Compare
|
Rebased and fixed some things and whatnot |
b26c971 to
3928357
Compare
jaime-jmebot
left a comment
There was a problem hiding this comment.
- The benchmark code is now isolated in its own
jme3-benchmarkmodule, addressing the requested separation. - The
LightListchanges and added tests look sound, including the retained-capacity and NaN sorting cases. - No further issues found in this review.
| tlist = list.clone(); | ||
| } else { | ||
| System.arraycopy(list, 0, tlist, 0, list.length); | ||
| System.arraycopy(list, 0, tlist, 0, listSize); |
There was a problem hiding this comment.
this can leave references to unused lights at the end of tlist when it shrinks, preventing them from being garbage collected.
Bulk-copy LightList updates and longer removal shifts. Use stable insertion sort up to 64 lights, with merge sort for larger lists and NaN distances.
Clear sorting scratch after every use, including exceptions, so removed lights are not retained. Compute distances before using scratch to handle nested world-light sorts. Filtered updates reserve space for accepted lights and use the parent's size after filter callbacks.
Measurements on this machine, master
6961525e→ this PR:All eight update cases improve over master. Ordinary-capacity re-sort intervals include both gains and regressions; retained-capacity stress cases improve. These re-sort measurements do not include clear/refill.
There are costs versus the previous PR: 256-light re-sorts execute about 5–7% more instructions with immediate scratch cleanup. The 257-light quarter-rejection update uses 13% more instructions while reducing reserved capacity from 1024 to 512 slots. Their timing differences remain inconclusive.
i7-11800H, Fedora 44, Temurin 25.0.4, JMH 1.37; CPU 2, one thread, 256 MiB heap. Six independent JVM groups in all six execution orders, 5×400 ms warmup and 6×300 ms measurement. Other work was running; dynamic clocks and contention limit timing claims. No FPS claim.
Core suite: 1,185 tests, zero failures, one skipped. Regression coverage includes shrinking/removing sorted lists, exceptions, nested sorts, NaNs, and parent-mutating filters. Both fixed-input render scenes match the published PR pixel for pixel.