Make the renderer ~1.3-1.5x faster: measure first, then remove work nothing consumes - #121
Merged
Conversation
Everything that follows is a performance change worth 1-20%, and this
repository had no way to tell such a change from machine noise. Two
properties of the setup make the obvious method wrong:
- Sequential comparison lies. Timing a change before and after, minutes
apart, reported a 12% *regression* for a change that bench_ab.sh then
showed to be a 4-5% improvement. The difference was background load.
bench_ab.sh alternates the two binaries within the same seconds so load
lands on both, and reports min and mean.
- The run-to-run spread reaches 15%, so a 1-5% step cannot be resolved by
timing at all. Those get counted in instructions via callgrind, which is
deterministic; the recipe is in bench_scenes.sh's header.
check_images.sh is the correctness gate: golden EXRs for every sample scene,
re-rendered and diffed. It renders at 16 spp deliberately.
min_samples_per_pixel defaults to 32 and the adaptive early-stop needs
taken >= min_spp, so at 16 every pixel takes exactly 16 samples; above it a
single-ulp difference changes a pixel's sample budget and cascades, which
would make a bit-identical change look structural.
Codegen, measured with the above:
- lto = "fat" over "thin": 3.989G instructions against 4.050G on cornellbox
-s 2 (-1.5%) for link time only, and the image stays bit-identical. Rust
does not enable fast-math and LLVM will not reassociate float arithmetic
without it, so whole-program inlining moves no bits.
- panic = "abort": no catch_unwind and no should_panic anywhere in the
workspace, and crust-core's "return Error, never exit" contract is about
recoverable errors, which are Result values. Removes the landing pads
around the bounce loop's expect()s; the binary drops 5.26 -> 4.36 MiB,
which callgrind cannot see because landing pads are not executed.
- No debug info. Callgrind names functions from the symbol table, which is
how every measurement here was taken; line tables would quadruple the
binary for detail nothing used. Ask per-invocation instead.
- No active target-cpu. .cargo/config.toml ships x86-64-v3 commented out:
reproducible across machines in a way `native` is not, and exactly the
AVX2+FMA level docs/simd.md already measured at 2-4%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bvh::hit and Bvh::hit_any each declared `[0u32; 3 * MAX_DEPTH + 4]` as their
traversal stack. The bound is genuinely worst-case — a wide node can advance
the binary depth by one level and leaves 3 deferred lanes behind — but the
array is 736 bytes and Rust zeroes it on entry, on every query, including
every instance descent, which re-enters hit recursively.
Callgrind attributed 180M of cornellbox's 187M memset instructions to exactly
those two declarations: 4.8% of the render, and 5.4% on veach_mis, all of it
clearing memory that is written before it is read.
Replaced by a small inline array plus a Vec that stays unallocated unless a
tree is deep enough to overflow it. The spill is the point: it makes
correctness unconditional, so STACK_INLINE becomes a tuning knob and the
"prove 3*MAX_DEPTH+4 is always enough" argument goes away rather than being
replaced by a smaller, shakier one. Threading a scratch buffer through the
query API instead was rejected: the leaf call makes traversal re-entrant, so
it would have to pass &mut through Prim::hit, every PrimNode arm, the public
Scene::intersect/occluded and every crust-core call site — and a thread-local
RefCell is not an escape, because the borrow is live across the leaf call and
the nested traversal would panic re-borrowing it.
STACK_INLINE = 32 is sized from measurement, not from the old bound. The
traversal-stats build now reports the deepest stack any query reached, and it
grows logarithmically as a good tree should:
primitives in one tree deepest stack
1 024 6
16 384 8
262 144 11
1 048 576 12
Instanced and nested layouts stay at 9-10, each level being its own tree with
its own stack. So 32 is ~2.7x the deepest figure seen on a million-primitive
tree. Deliberately not tightened to hug that: overflowing costs a heap
allocation per traversal, which would be worse than the memset this removes.
Shrinking to 16 measured ~0.3% — not worth moving that cliff closer.
memset falls from 186.8M instructions to 6.8M (the remainder is EXR/PNG output
and USD parsing, which was always there); total falls 2.7%, and wall clock
3-6% on cornellbox, veach_mis and instancing. Output is bit-identical on all
14 sample scenes and the SIMD matrix is unchanged. The stack type is unit
tested directly — push/pop across the spill boundary, and interleaved — rather
than by trying to build a tree deep enough to reach it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
eval_all evaluated diffuse, specular, coat and fuzz unconditionally and applied
each lobe's weight only as a final multiply. With the default material —
coat_weight 0, fuzz_weight 0, base_metalness 0 — that meant computing, per
call, a full anisotropic GGX D + Smith G2 with two sqrts for a coat that
contributes nothing, the entire F82 metal Fresnel chain for a surface that is
not metal, and sheen_charlie, which holds the only unconditional powf on the
default shading path.
Guards added in eval_all (coat, fuzz), eval_specular (the two ends of the
metalness blend) and eval_diffuse (the presence weights).
Bit-identical, and that is a proof rather than a hope: every skipped lobe ends
in a multiply by its weight, and every one is finite for all inputs, because
every GGX denominator is floored (roughness_to_alpha_aniso clamps alpha >= 1e-4,
the cosines at 1e-4, so D <= ~3e23) and sheen_charlie is bounded because
sheen_charlie_d clamps alpha >= 0.05, keeping its powf argument in [0,1].
fresnel_f82_tint clamps to [0,1]. A finite value times 0.0 is exactly +0.0, and
+0.0 + x == x. The one way it could have differed is a lobe overflowing to inf,
where inf * 0.0 = NaN today and 0.0 after — unreachable given the clamps, and
stated in the code rather than left implied.
eval_all falls from 367M instructions to 250M (-32%), the shading powf
disappears (libm 85.1M -> 40.3M, the remainder being tone mapping in main), and
total falls 4.6% — 6-8% wall clock on cornellbox, veach_mis and
openpbr_showcase. Verified against openpbr_showcase in particular, which is the
only sample scene authoring non-zero coat, fuzz, thin film, metalness and
dispersion, i.e. the only one that exercises the guards' taken branches.
Two notes recorded in passing, neither acted on here:
- pdf_all's coat term is *not* skippable the way eval_coat is.
LobePmf::from_params floors an absent lobe's selection weight at 1e-6, so
p_coat is nonzero and the term genuinely belongs in the density sampling
divides by. Deduplicating pdf_diffuse and pdf_fuzz, which are the same
expression, measured exactly nothing — LLVM had already done it.
- Those floors are a latent variance bug, documented at from_params. At
coat_weight == 0 the coat lobe still holds ~1e-6 of the selection mass, and
because the default coat_roughness of 0 floors its alpha at 1e-4, pdf_coat
reaches ~3e7 near the mirror direction. So a zero-energy lobe is picked one
sample in a million and inflates the mixture density every specular sample
divides by. Unbiased, but it wastes samples and slightly darkens
near-mirror specular. Fixing it changes the image, so it wants a converged
A/B of its own rather than being folded in here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three changes to stop the integrator and importer doing work nothing consumes.
They share one file each way, so they land together.
1. Bake single-placement meshes flat, instead of instancing everything.
emit_mesh wrapped every UsdGeomMesh in a Geometry::Instance around its own
single-mesh scene, whether or not that geometry was shared; flat world-space
triangles were only the fallback for a non-invertible transform. So a five-mesh
Cornell box performed 3.85 instance descents per camera ray, each one a ray
transform, a fresh slab and shear setup, and a cold descent into a second tree
— and the top-level BVH never saw a triangle, only loose boxes-of-boxes that
spatial splits cannot tighten, since InstancePrim does not override
clipped_aabb. Instancing something placed once buys no sharing to amortise any
of that against.
The decision is deferred, because a placement count is only final once the
whole stage is walked: emit_mesh interns the triangles by content and reserves
a geom_id, and flush_meshes fills the slots in afterwards. Reserving matters —
geom_ids are handed out in traversal order and index the material table, and
lights take theirs from the same counter, so deciding late must not renumber
them. Hence SceneBuilder::set_geometry and WorldBuilder::reserve_slot.
Deciding globally rather than per chunk is load-bearing twice over: cornellbox
streams, so per-chunk every mesh would look like a singleton and results would
differ under CRUST_STREAM_IMPORT=0; and geometry referenced from two elements
of a production stage would get one resident copy per element, a regression in
exactly the case sharing exists for. Genuine instancing is untouched —
collect_proto_parts commits a prototype's scene as before, which is also what
marks that mesh ineligible for baking, so a mesh used both directly and as a
prototype is not stored twice.
Baking a mirrored (det < 0) placement swaps two indices. The instanced path
derives its normal inside the prototype and maps it out through the inverse
transpose; baking derives it from world-space vertices, and those disagree in
sign, so without the swap front_face inverts and every mirrored prim renders
inside-out — flipping which side of a refractive interface a ray thinks it is
on. No sample scene has a mirror, so this is pinned by unit tests that compare
baked against instanced hits directly; both fail if the swap is removed.
2. Do not sample the shutter when nothing moves.
draw_sample_f32::<N> computes a whole 4-dimensional Owen-scrambled Sobol block
whatever N is, and the integrator drew one per camera ray for a single float —
4.2% of the render on a scene with no motion blur. ray.time is read by exactly
one thing, InstancePrim::transforms_at, which ignores it when the instance is
static, so on such a scene every value produces the same image. Scene now
carries a has_motion flag folded in at commit (propagating out of nested
instances, which is the one way to get this wrong and silently drop blur — hence
a test for it), and the draw is skipped when it is false. Not calling
new_domain cannot perturb the other dimensions: it is a pure function of the
parent state taken by &self.
3. Reuse the path's vertex buffer.
trace_path allocated a Vec<VertexRec> per camera sample: one malloc/free pair
per sample, 4.4% of the render. Now borrowed from a PathScratch owned per work
unit — the same ownership story as RayStats, private to one thread by
construction. The allocator disappears from the profile entirely.
Also fixed while measuring this: SceneBuilder::commit grew the primitive array
by repeated doubling, which was invisible until baking pushed hundreds of
thousands of triangles through it and left +28 MiB of genuinely committed
capacity (MemoryFootprint counts capacity, not length, which is why it showed
up). Sized exactly once from the geometries now.
Measured, pristine baseline to here, interleaved:
veach_mis 4.060s -> 2.770s -31.8% 56.9 -> 89.3 Mray/s
cornellbox 1.382s -> 1.021s -26.1% 39.5 -> 55.5 Mray/s
Kitchen_set 3.624s -> 2.847s -21.4%
instancing 0.341s -> 0.286s -16.1%
nested_instancing 0.347s -> 0.293s -15.6%
openpbr_showcase 0.682s -> 0.579s -15.1%
Instructions on cornellbox -s 2: 4.050G -> 2.906G, -28.2%. Instance descents
per camera ray: 3.85 -> 0.13. Total node visits actually rise slightly
(6.63 -> 7.59, one deeper tree) — what was costing was the descents, not the
nodes.
Items 2 and 3 are bit-identical on all 14 sample scenes, and so is item 1 under
CRUST_MESH_BAKE=0, which is what separates a deferral bug from a baking
difference. Baking itself does change output, because a different top-level BVH
orders exact ties differently. That it is noise and not bias is checked by
convergence, not asserted: on veach_mis the mean absolute difference falls
2.96e-4 -> 4.83e-5 -> 2.61e-5 at 16, 1024 and 4096 spp, i.e. 1/sqrt(N).
Known trade-off, documented in CLAUDE.md: resident memory is neutral
(Kitchen_set kernel memory 134.59 -> 134.44 MiB) but the *transient* build peak
is higher, 374 -> 579 MiB, because one SBVH build over ~600K references holds a
few hundred MiB where 1788 small builds held almost nothing. PrimRef and the
binary Node are 48 bytes each and merge concatenates child arrays while both
children live. That is pre-existing builder behaviour this change exercises
rather than introduces; the lever, if that peak ever outweighs the ~20% render
win, is a triangle-count cap on baking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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
1 participant
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.
The renderer was slow in a way the existing profiler could see but not explain:
samples/cornellbox.usda— 1 582 triangles — managed 39.5 Mray/s across 48 threads, about 2 600 cycles per ray query, on a scene whose BVH should traverse in a few hundred.An instruction-level profile (callgrind) said the time was not going where anyone had assumed. Four of the five largest costs were work that produced nothing.
Result
Interleaved A/B against the pristine baseline:
Instructions on cornellbox
-s 2: 4.050 G → 2.906 G (-28.2%).What was actually costing
UsdGeomMeshwas wrapped in an instance, shared or not, so a 5-mesh Cornell box did 3.85 instance descents per camera ray — each a ray transform, a fresh slab/shear setup and a cold descent into a second tree — and the top-level BVH never saw a triangle, only boxes-of-boxes that spatial splits cannot tighten. Now 0.13/ray. This is the same pathology the previous commit found on the Moana island, and it confirms that commit's conclusion that the fix belonged in the importer rather than the kernel.memsetinstructions, written before read.ray.timeis consumed only by a moving instance.malloc/freepair per camera sample for the path's vertex buffer (4.4%).powfin shading.eval_all-32%.Correctness
219 tests pass (8 new), SIMD matrix passes under all four codegens, no new clippy warnings, each commit compiles standalone.
Commits 1-3 are bit-identical on all 14 sample scenes, and so is commit 4 under
CRUST_MESH_BAKE=0— which is what separates a deferral bug from a baking difference.Baking does change output, because a different top-level BVH orders exact ties differently. That it is noise and not bias is checked, not asserted — on veach_mis the mean absolute difference falls 2.96e-4 → 4.83e-5 → 2.61e-5 at 16 / 1024 / 4096 spp, i.e. 1/√N.
The mirrored-transform case (
det < 0) needs a winding swap orfront_faceinverts and mirrored prims render inside-out, flipping which side of a refractive interface a ray thinks it is on. No sample scene has a mirror, so it is pinned by unit tests comparing baked against instanced hits directly; both fail if the swap is removed.Trade-off you should know about
Resident memory is neutral (Kitchen_set kernel memory 134.59 → 134.44 MiB), but only after fixing a second bug this surfaced:
commit()grew the primitive array by repeated doubling, costing +28 MiB of genuinely committed capacity once meshes were baked into it.What is not fixed is the transient build peak: 374 → 579 MiB on Kitchen_set, because one SBVH build over ~600 K references holds a few hundred MiB where 1 788 small builds held almost nothing (
PrimRefand the binaryNodeare 48 bytes each, andmergeconcatenates child arrays while both children live). Pre-existing builder behaviour this change exercises rather than introduces. Documented inCLAUDE.md; the lever, if that peak ever outweighs the ~20% render win, is a triangle-count cap on baking.Also here
scripts/bench_ab.shalternates two binaries within the same seconds;check_images.shgates correctness at 16 spp (above it, adaptive sampling turns a 1-ulp difference into a different sample budget and a bit-identical change looks structural).CLAUDE.mdgains a "Measuring a change" section.lto = "fat"(-1.5% instructions, bit-identical),panic = "abort"(binary 5.26 → 4.36 MiB), and a commented-outx86-64-v3opt-in rather than a non-distributabletarget-cpu=native.LobePmf::from_paramsfloors absent lobe weights at 1e-6, so a material withcoat_weight == 0still selects the coat lobe one sample in a million and inflates the mixture density every specular sample divides by (defaultcoat_roughnessof 0 floors its α at 1e-4, sopdf_coatreaches ~3e7 near the mirror direction). Unbiased, but it wastes samples and darkens near-mirror specular. Fixing it changes the image, so it wants a converged A/B of its own.Deliberately deferred
Two planned changes were dropped on evidence: hoisting OpenPBR's per-material derived values predicted ~1% for a refactor across ~10 functions and ~20 test sites, and the light-list indices measure ~0 on these scenes (cornellbox has no light-list lights at all). Both remain worth doing — just not ahead of the traversal work.
docs/simd.mdnow notes that BVH8/node compression should be re-measured before starting: the counters previously pointed at node visits, but most of those visits should never have happened.🤖 Generated with Claude Code