You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
UniformSphericalRandom promised unit vectors uniformly distributed over
the sphere surface (used by LightBulb and PlanarLight for Monte Carlo
light sampling), but sampled the polar coordinate as an angle uniform in
[0, 2*PI]. That concentrates samples near the poles instead of spreading
them evenly over the surface.
Sample cos(theta) uniformly in [-1, 1] instead, deriving sin(theta) from
it, so every direction on the sphere is equally likely. Vectors remain
unit length.
Add UniformSphericalRandomTest, which draws many samples and asserts that
each squared coordinate averages to 1/3 (the symmetric value for a true
uniform sphere). Before the fix the mean of x^2 was ~1/4; the test now
passes.
UniformSphericalRandom promised unit vectors uniformly distributed over
the sphere surface (used by LightBulb and PlanarLight for Monte Carlo
light sampling), but sampled the polar coordinate as an angle uniform in
[0, 2*PI]. That concentrates samples near the poles instead of spreading
them evenly over the surface.
Sample cos(theta) uniformly in [-1, 1] instead, deriving sin(theta) from
it, so every direction on the sphere is equally likely. Vectors remain
unit length.
Add UniformSphericalRandomTest, which draws many samples and asserts that
each squared coordinate averages to 1/3 (the symmetric value for a true
uniform sphere). Before the fix the mean of x^2 was ~1/4; the test now
passes.
The class-level javadoc still described the pre-fix algorithm ("random
azimuth and polar angles"), which is exactly the non-uniform sampling that
was corrected. Reword it to state that the polar coordinate is chosen so
that cos(theta) is uniform on [-1, 1], matching the corrected implementation
and the inline comment in evaluate().
The real-time audio render path accumulated native/direct memory across
renders in a JVM, which starved the device allocator and made successive
renders fail: an OutOfMemoryError on a memory-tight host, and silent output
(peak amplitude 0.0) once a GPU device allocator is exhausted. This is the
cause of the test-media-mac (Metal) failures of
AudioSceneRealTimeCorrectnessTest.realTimeProducesAudio and
multiBufferWithEffects, where the first render in a shard produced audio and
every later render in the same JVM came back silent.
Each render allocated two sizable resources that were never released: a
WaveOutput with a full-timeline capture buffer (~162 MB for the default
stereo timeline) and a real-time runner owning a PatternRenderStream
(render-ahead ring plus a daemon producer thread) and a compiled mixdown
model. The runner's TemporalCellular exposed no way to release any of it.
Changes:
- AudioSceneRealtimeRunner: the CellList and PDSL runners are now named local
classes implementing Destroyable. destroy() stops the render-ahead producer
thread and frees the ring, the compiled mixdown model, and the per-buffer
frame index (and, on the CellList path, the per-frame CellList). The
scene-owned render buffers are left to the scene.
- RealTimeTestHelper: renderRealTime now destroys the runner and WaveOutput in
a finally block (so a failed render still releases its memory), and
createSceneWithWorkingSeed destroys the throwaway seed-search scene.
Verified on a memory-constrained host: the five-method real-time sequence that
previously errored with "Cannot reserve 162288000 bytes of direct buffer
memory" now runs with every render non-silent (rms 0.22-0.40) and zero
failures, with no new checkstyle, code-policy, test-timeout, or duplicate-code
violations.
The remaining test-media-mac failures of optimizerScoresCarryStems and
generateAudioFile are a separate infrastructure issue: AudioSceneTestBase's
requireCuratedLibrary deliberately fails when a GPU is present but the curated
sample library (/Users/Shared/Music/Samples) is not mounted on the runner.
That is unchanged base-branch behavior and is not addressed here.
Each sample is a native-memory-backed Vector, but the loop retains cleanup to GC/finalization. With 500,000 iterations this can exhaust the hardware allocator before the statistical assertions run. Close each vector after accumulating its coordinates.
Addresses review findings on the real-time render fix and the sphere-sampling
regression test. All changes are confined to test and test-helper code.
- UniformSphericalRandomTest: each sampled Vector is native-memory-backed
(Vector extends PackedCollection); the 500k-sample loop is now wrapped in
try-with-resources so each sample is released immediately and the hardware
allocator is not exhausted before the statistical assertions run.
- RealTimeTestHelper.createSceneWithWorkingSeed: the seed-scoring scene is now
destroyed in a finally, so it is released even if the seed search throws.
- RealTimeTestHelper.renderRealTime: the WaveOutput is allocated before the
runner, so a throw from runnerRealTime would leak it; the two try blocks are
now nested (inner finally destroys the runner, outer finally destroys the
output) so the output buffer is freed even when runner construction throws.
Addresses the three native-memory leaks Copilot flagged on the latest
real-time render cleanup, all the same leak class this branch targets
(per-render accumulation starving the device allocator):
- AudioSceneRealtimeRunner.PdslRunner.destroy() now releases the mixdown
argument buffers built by MixdownManagerPdslAdapter.buildArgsMap().
These (delay, feedback, bus, reverb, automation, and stem collections)
are supplied to the model as external collection providers, so
CompiledModel.destroy() does not reclaim them; they are destroyed after
the compiled model and the map is cleared.
- RealTimeTestHelper.renderRealTime() now retains and destroys the
compiled setup, tick, and write runnables returned by OperationList.get()
(AcceleratedOperation/OperationListRunner own native kernels the runner
does not retain), alongside the runner, in the finally block.
Verified: build validator passed (checkstyle, code_policy, test_timeouts,
duplicate_code, invalid_files) and AudioSceneRealTimeCorrectnessTest
realTimeFrameAdvancement, frameRangeWithEffects, and multiBufferWithEffects
pass (3 run, 0 failures), exercising both cleanup paths.
Review follow-up on the real-time render native-memory cleanup.
- AudioSceneRealtimeRunner: PdslRunner.destroy() now releases the compiled
pattern render operation it creates. PatternRenderStream treats its
constructor arguments as borrowed (it frees only the ring and slot copies it
allocates), so the render op is destroyed by its owner, after the stream has
stopped its producer thread. Fixes the remaining Copilot leak finding.
- WaveOutput: destroy() now releases the WaveData buffer when the output
allocated it itself. The per-channel entries are range views into that buffer,
and destroying a delegated view does not free the root, so each real-time
render leaked ~162 MB. Caller-supplied data/collections are left untouched
(new private owned-aware constructor; caller-facing constructors pass
owned=false).
- AudioSceneTestBase: an @after now destroys scenes the tests create via
AudioScene.destroyAll(). The real-time tests build a scene per method and
never released it; with surefire reusing one JVM per module these accumulated
and exhausted the direct-buffer allocator.
- WaveOutputWriteTest: adds coverage for both ownership branches of WaveOutput
destruction (caller-owned buffer preserved; owned timeline buffer released and
idempotent).
Verified: build validator (checkstyle, code_policy, test_timeouts,
duplicate_code, invalid_files) all pass; the new WaveOutput tests and the
existing writeMultipleBatches pass; the full AudioSceneRealTimeCorrectnessTest
class passes under the CI direct-memory limit (8g), where it previously
exhausted the local 1g test-runner cap.
This still leaks the compiled model's output collection. CompiledModel.compile() allocates the collection returned as masterOutput, while CompiledModel.destroy() only destroys its forward/backward operations; this local is its only lifecycle owner. Destroy masterOutput after the compiled operation so every runner releases that device buffer.
Runner cleanup leaks the combined-effects stems collection
The cleanup omits the fxPackedCollection allocated for combined effects stems at line 482. It is captured by stemAppends but is not an argument-map value, and computation operations do not own externally supplied collections, so a stems-active runner still leaks one native buffer per construction. Retain that collection as runner-owned state and destroy it here.
analyzeAudio() leaks the loaded WaveData collection
This call still leaves native memory behind: analyzeAudio() loads a WaveData and returns scalar statistics without destroying the loaded collection. Repeated renders therefore replace the freed timeline with another live native allocation. Make analyzeAudio() use try-with-resources around WaveData.load(...).
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed. PatternRenderStream.stop() joined the producer for only 2s and then set producer = null unconditionally, so a render longer than that would leave the producer alive while destroy() went on to free the ring, the slot copies, and the caller then freed renderOp — a use-after-free, and start() could also spawn a second producer beside the lingering one.
stop() now confirms termination before returning: it clears running, interrupts the producer, and then join()s in a loop until the thread is no longer alive (re-interrupting the caller afterward if its own wait was interrupted). Because running == false plus the interrupt unblocks the producer from empty.acquire(), the loop observes !running after at most one more render iteration, so the wait is bounded in practice by a single buffer render — it does not hang. With this, destroy() and the caller's renderOp release only ever touch resources the producer has stopped using. Verified: AudioScenePdslCutoverTest#pdslVsCellListPlumbingSmoke (full PDSL render + runner destroy) and the effects-path real-time correctness tests pass.
Addresses the latest review findings on the real-time render lifecycle work:
- PatternRenderStream.stop() now waits for the producer thread to actually
terminate (loop join after interrupt) instead of abandoning it after a
bounded 2s join. destroy() frees the ring and the caller frees renderOp
next, so a producer still alive would use them after release.
- PdslRunner.destroy() now releases the compiled model's output collection
(masterOutput, which CompiledModel.destroy() does not reclaim) and the
combined-effects stems buffer (retained as runner-owned state), closing two
per-render native buffer leaks.
- RealTimeTestHelper.analyzeAudio() wraps WaveData.load() in
try-with-resources so each analysis releases its decoded timeline.
- WaveOutput exposes getOwnedBuffer() so the owned-timeline test can assert
the backing buffer is actually destroyed; the previous test only checked
that a double destroy did not throw and passed even without the fix.
CompiledModel.destroy() currently releases only its forward and backward operations; the setup runnable created by CompiledModel.compile() is not released. Since this runner compiles a new model per construction, repeated renders still retain one compiled setup operation each. Extend CompiledModel.destroy() to destroy its setup runnable so this cleanup actually releases every compiled model operation.
Two native-memory-leak fixes surfaced by review of the real-time render
lifecycle work on this branch:
- CompiledModel.destroy() now releases the compiled one-time setup
operation alongside the forward and backward passes. compile() builds
setup from Model.setup() into a Runnable owning its own kernels, and the
CompiledModel is its only lifecycle owner; reset() is its only other
caller and the model is dead after destroy, so releasing it here reclaims
those kernels for every caller that compiles a fresh model per unit of
work (the per-render PDSL runner among them).
- AudioSceneRealtimeRunner.createPdsl() now rolls back on construction
failure. The runner-owned native resources (frame index, render
operation, render-ahead ring, argument buffers, compiled model, its
output buffer, and the combined-effects stems buffer) are released by the
returned runner's destroy(), but that owner is not built until every
allocation has succeeded. A throw before then (PDSL parse/compile, the
throwaway forward(), or output/stem wiring) previously leaked everything
already allocated. Each resource is now tracked as it is created and
released in reverse order if construction fails, then the throwable is
rethrown unchanged.
Verified: mvn -pl studio/compose -am install -DskipTests (clean rebuild of
the chain); LayerTrackingTest 4/0/0 (exercises CompiledModel compile +
destroy); AudioScenePdslCutoverTest#pdslVsCellListPlumbingSmoke 1/0/0
(exercises createPdsl construction, render, and destroy); build validator
checkstyle/code_policy/javadoc all 0 violations.
The test documents that the caller owns the CompiledModel output handle
(destroy() releases only the compiled operations), yet it never released
that handle or its input collection, leaking native memory in the shared
test JVM. The input is now closed via try-with-resources and the output
handle is destroyed after the compiled operations.
If compiled.run() throws and Destroyable.destroy(compiled) also throws, the finally exception replaces the actual compilation/execution failure. This contradicts the rollback behavior added elsewhere in this PR and loses the primary diagnostic. Catch the primary failure and use destroyAll(primary, compiled) so cleanup failures are suppressed onto it; on the success path, destroy normally.
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed with the second option you suggested: the scene now owns the runners it creates. AudioSceneRealtimeRunner tracks every runner built by create(...) until that runner is destroyed, and it is now Destroyable itself. Its destroy() releases the runners that are still live, newest first, with failures aggregated. AudioScene keeps a single instance of it and calls realtimeRunners.destroy() at the top of destroy(). A runner a test never released therefore gets its PatternRenderStream stopped and joined, and its ring, render op, model and argument buffers freed, before the scene frees the render cells and consolidated buffer that the producer renders into. Runner destroy() is now idempotent (the runner untracks itself), so a runner the caller already destroyed is not released a second time. New AudioSceneRunnerOwnershipTest checks two things. First, the pattern-render-ahead thread that setup() starts is gone after scene.destroy(), and a later caller destroy() does nothing. Second, live-runner tracking and idempotence hold on both the PDSL and CellList paths. Both tests pass, as do pdslVsCellListPlumbingSmoke and renderBufferConsolidation; checkstyle, code_policy, test_timeouts and duplicate_code are clean.
The per-test AudioScene.destroyAll() teardown could free a scene's render
cells and consolidated buffer while a PDSL runner the test never destroyed
still had its render-ahead producer thread running, a use-after-free that
also leaked the runner's ring, model and argument buffers.
AudioSceneRealtimeRunner now tracks every runner it builds until that
runner is destroyed, and is itself Destroyable: destroy() releases the
runners still live, newest first. Runner destroy() is idempotent, so a
runner its caller already released is not released again. AudioScene keeps
one collaborator instance and destroys it first in destroy(), stopping and
joining producer threads before freeing the buffers they render into.
AudioSceneRunnerOwnershipTest covers scene-driven shutdown of an
undestroyed PDSL runner and live-runner tracking across both DSP paths.
Adding setup to the same sequential teardown means a failure while destroying forward or backward prevents the newly owned setup kernels from ever being released. Since this method owns independent compiled operations, use Destroyable.releaseAll so one cleanup failure does not leak the rest.
Several destroy() paths released independent native-kernel and buffer
owners as a plain sequence, so a throw from one release left the
remaining resources allocated. Route each through
Destroyable.releaseAll so every release is attempted best-effort and
the first failure is rethrown (later ones suppressed onto it).
- CompiledModel.destroy(): forward, backward, and setup are independent
compiled operations; a throw destroying forward previously leaked
backward and setup.
- AudioScene.destroy(): a runner-cleanup failure (which
AudioSceneRealtimeRunner.destroy() rethrows after attempting every
runner) no longer skips the scene's own teardown or its removal from
the active-instance registry. Runners stay first so their producer
threads stop before the buffers they render into are freed.
- AudioSceneRealtimeRunner CellListRunner.destroy() and
PdslRunner.destroy(): a throw from an earlier release no longer leaves
the later resources allocated after the runner has already untracked
itself (making further destroy() a no-op).
AudioSceneRunnerOwnershipTest now destroys the self-allocating
WaveOutputs and the compiled setup runnable it creates, closing the
shared-JVM native-memory leak those tests would otherwise introduce.
Verified: LayersTests#compiledModelOutputHandle passes; checkstyle,
code_policy, test_timeouts, and duplicate_code are clean.
The newly owned backing buffer is not released if destruction of any writer or channel producer throws, because the method exits before reaching this block. Native cleanup failures are explicitly possible elsewhere in this change, so teardown must attempt every independently owned resource; otherwise the large timeline buffer remains allocated permanently.
Register allocations immediately to prevent rollback leaks
This rollback only reaches values already inserted into args, but two allocation pairs still keep the first collection in a local until the second allocation succeeds: buffers/heads at lines 508–511 and reverbBuffers/reverbHeads at lines 962–965. If the second allocation fails under the memory pressure this path handles, the first collection is absent from the map and leaks. Insert each collection immediately after allocating it, as is already done for the response tables.
Addresses the remaining native-memory cleanup findings from the latest
PR review round:
- AudioScene: a CellList real-time runner shares its cells with the
scene (getCells returns the same instance it stores as activeCells),
so the runner destroying the cells and the scene destroying activeCells
double-traversed the cell graph. Add AudioScene.destroyActiveCells,
which releases a cell list only when it is still the current active
list and clears the reference atomically; the runner and scene teardown
both route through it, so the graph is freed exactly once. This also
skips a cell list a later getCells already replaced.
- AudioSceneRealtimeRunner (PdslRunner): release each argument buffer as
its own best-effort action so a value whose destroy() throws can no
longer skip the remaining buffers; the map is cleared last.
- WaveOutput: run the writers, channel producers, and self-allocated
backing buffer as independent best-effort release actions so a writer
or producer failure cannot leave the large timeline buffer allocated.
- MixdownManagerPdslAdapter: register the feedforward and reverb ring
buffers/heads in the argument map immediately after allocation so the
build-failure rollback can free the first collection if the second
allocation throws.
Adds a regression test asserting destroyActiveCells leaves a cell list
that is not the scene's active list untouched.
AudioScene.destroyActiveCells now clears activeCells before calling the
fallible CellList.destroy(), so a release that throws (for example from a
runner's destroy()) no longer leaves the scene holding the list for its own
teardown to traverse again. getCells and the PDSL prepare path reuse
destroyActiveCells instead of duplicating the clear-and-destroy block.
The CellList runner's construction-failure rollback now releases its cells
through scene.destroyActiveCells rather than destroying the raw list, so a
failure after getCells no longer leaves the scene referencing a destroyed
list.
Adds AudioSceneRunnerOwnershipTest#failedActiveCellsReleaseIsNotRepeatedBySceneDestroy,
which fails without the reordering.
AudioScene.renderChannel built a real-time runner through runnerRealTime,
which tracks every runner strongly until it is destroyed or the scene is,
but only reset() the runner and output in its finally block. Repeated
renders on a long-lived scene therefore accumulated each runner's ring,
model, and argument buffers (and the WaveOutput timeline buffer) in the
live-runner tracker until the whole scene was destroyed.
renderChannel now owns the runner it builds: it releases the compiled
setup/tick/write operations, the runner, and the WaveOutput through
Destroyable.releaseAll when the render finishes, so nothing is left in the
tracker. Added AudioScene.getLiveRunnerCount() and a regression test,
renderChannelReleasesRunnerEachCall, asserting the live count stays at zero
across successive renders.
If compiled.run() throws and compiled.destroy() also throws, Java's finally semantics replace the execution failure with the cleanup failure. That contradicts the rollback behavior used elsewhere and hides the actual build error. Catch the primary failure and use destroyAll(primary, compiled) so cleanup failures are suppressed onto it; on the success path, destroy normally.
The reason will be displayed to describe this comment to others. Learn more.
You're right, and this is now fixed. The render moved into AudioSceneRealtimeRunner.render(channel, frames, outputPath, bufferSize), next to the runner it builds and tracks, and AudioScene.renderChannel now delegates to it. The WaveOutput, the runner, and the compiled setup, tick and write ops are each pushed onto an ownership deque as soon as they are created, inside the guarded scope.
On failure (runner build, op compilation, or the render itself): Destroyable.destroyAll(e, owned) releases everything built so far, newest first, and then the original error is rethrown with any release failures suppressed onto it.
On success:releaseAll releases the same resources newest first, so the compiled ops go before the runner and output buffers they read.
New regression test AudioSceneRunnerOwnershipTest#renderReleasesRunnerWhenConstructionFails wraps the built runner so that tick().get() throws after the runner exists and setup has compiled. It asserts that the original exception propagates and the live-runner count is 0. The class runs 6 tests with 0 failures, and checkstyle, code_policy, test_timeouts and duplicate_code are all clean.
AudioScene.renderChannel built the real-time runner and compiled its
setup/tick/write operations before entering the guarded scope, so a
failure while building the runner or compiling an operation left the
runner tracked as live and its buffers, the compiled kernels, and the
WaveOutput timeline buffer allocated.
The render now lives in AudioSceneRealtimeRunner.render, next to the
runner it builds and tracks. Each resource is registered as soon as it
is created; on failure everything built so far is released and the
original error is rethrown with release failures suppressed onto it,
and on success the resources are released newest first. renderChannel
delegates to it, which also brings AudioScene.java back under the
1500-line recommendation.
Adds AudioSceneRunnerOwnershipTest#renderReleasesRunnerWhenConstructionFails,
which makes tick compilation throw after the runner exists and asserts
the failure propagates and no runner remains live.
AudioScene.destroyAll() iterates scenes with a plain loop and aborts as soon as one scene.destroy() rethrows a cleanup failure. Because this new @After is specifically responsible for releasing every scene accumulated by a test, one failing scene leaves all later registry entries and their native memory live for subsequent tests. Make destroyAll() aggregate each scene destruction with Destroyable.releaseAll, as the per-scene teardown already does.
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
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.
UniformSphericalRandom promised unit vectors uniformly distributed over
the sphere surface (used by LightBulb and PlanarLight for Monte Carlo
light sampling), but sampled the polar coordinate as an angle uniform in
[0, 2*PI]. That concentrates samples near the poles instead of spreading
them evenly over the surface.
Sample cos(theta) uniformly in [-1, 1] instead, deriving sin(theta) from
it, so every direction on the sphere is equally likely. Vectors remain
unit length.
Add UniformSphericalRandomTest, which draws many samples and asserts that
each squared coordinate averages to 1/3 (the symmetric value for a true
uniform sphere). Before the fix the mean of x^2 was ~1/4; the test now
passes.