Improve robustness and memory usage when loading many meshes#9684
Open
hotzenklotz wants to merge 3 commits into
Open
Improve robustness and memory usage when loading many meshes#9684hotzenklotz wants to merge 3 commits into
hotzenklotz wants to merge 3 commits into
Conversation
- Dispose geometries and materials when meshes are removed from the scene so that their GPU buffers are actually freed (also frees the eagerly added chunk meshes that are replaced by the merged geometry). - Limit how many segments are meshed simultaneously via a token channel so that the first meshes finish early and the memory pressure of in-flight chunk buffers stays bounded. - Handle failures of mergeGeometries/BVH computation (e.g., when buffers cannot be allocated under memory pressure) gracefully by keeping the unmerged chunk meshes as a fallback. - Yield to the event loop after each decoded chunk to avoid a tight loop that starves rendering and can silently stop the saga middleware (redux-saga#1592). - Don't show an error toast for each failed (and retried) mesh chunk request; the final error is still surfaced by the calling saga. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR optimizes precomputed mesh loading by throttling concurrency to 4 segments, yielding event loop control during chunk processing, gracefully handling merge failures, and ensuring GPU memory cleanup on mesh removal. Request retry behavior documentation and changelog entries document these improvements. ChangesMesh Loading Optimization and Resource Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts`:
- Around line 502-513: The catch block currently sets mergedGeometry = null
without disposing the BufferGeometry returned by mergeGeometries, causing a GPU
leak when computeBvhAsync throws; update the catch to check if mergedGeometry is
non-null and call mergedGeometry.dispose() (and also dispose
mergedGeometry.boundsTree if applicable) before setting mergedGeometry = null so
the merged BufferGeometry and any associated BVH resources are released;
reference the mergedGeometry variable and the functions mergeGeometries and
computeBvhAsync to locate the code to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dc9b86a-384b-49a8-b3fd-ae3786e1b719
📒 Files selected for processing (6)
CLAUDE.mdfrontend/javascripts/admin/api/mesh.tsfrontend/javascripts/viewer/constants.tsfrontend/javascripts/viewer/controller/segment_mesh_controller.tsfrontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.tsunreleased_changes/9684.md
Comment on lines
+502
to
+513
| try { | ||
| mergedGeometry = ( | ||
| sortedBufferGeometries.length > 0 ? mergeGeometries(sortedBufferGeometries, false) : null | ||
| ) as BufferGeometryWithInfo | null; | ||
| if (mergedGeometry != null) { | ||
| mergedGeometry.vertexSegmentMapping = new VertexSegmentMapping(sortedBufferGeometries); | ||
| mergedGeometry.boundsTree = yield* call(computeBvhAsync, mergedGeometry); | ||
| } | ||
| } catch (exception) { | ||
| mergedGeometry = null; | ||
| console.error(`Failed to merge mesh chunks for segment ${segmentId}:`, exception); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts"
# Show the exact section around the reported lines
sed -n '450,560p' "$FILE" | nl -ba
# Locate any other disposal/cleanup of mergedGeometry or buffer geometries
rg -n "mergedGeometry|dispose\(|BufferGeometryWithInfo" "$FILE"
# Find definition of BufferGeometryWithInfo (type/interface/class)
rg -n "type BufferGeometryWithInfo|interface BufferGeometryWithInfo|class BufferGeometryWithInfo|BufferGeometryWithInfo" frontend/javascripts/viewer -S
# Find computeBvhAsync implementation/usage
rg -n "computeBvhAsync" frontend/javascripts/viewer -S
# Find mergeGeometries usage and whether callers dispose merged outputs
rg -n "mergeGeometries\(" frontend/javascripts/viewer -SRepository: scalableminds/webknossos
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts"
# Section around the reported lines (with awk line numbers)
sed -n '450,560p' "$FILE" | awk '{printf "%6d\t%s\n", NR+449, $0}'
echo "----"
# Locate disposal/cleanup related to mergedGeometry
rg -n "mergedGeometry|dispose\(|new VertexSegmentMapping|boundsTree" "$FILE"
echo "----"
# Find BufferGeometryWithInfo definition
rg -n "type BufferGeometryWithInfo|interface BufferGeometryWithInfo|class BufferGeometryWithInfo|BufferGeometryWithInfo" frontend/javascripts/viewer -S
echo "----"
# Find computeBvhAsync definition/usages
rg -n "function\s+computeBvhAsync|computeBvhAsync\s*\(" frontend/javascripts/viewer -S
echo "----"
# Find mergeGeometries definition/usages
rg -n "function\s+mergeGeometries|mergeGeometries\s*\(" frontend/javascripts/viewer -SRepository: scalableminds/webknossos
Length of output: 7256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_SAGA="frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts"
FILE_HELPERS="frontend/javascripts/viewer/controller/mesh_helpers.ts"
FILE_CONTROLLER="frontend/javascripts/viewer/controller/segment_mesh_controller.ts"
echo "---- mesh_helpers.ts (computeBvhAsync, types)"
sed -n '1,220p' "$FILE_HELPERS" | awk '{print NR "\t" $0}'
echo "---- segment_mesh_controller.ts (addMeshFromGeometry/removeMeshById)"
# Print a focused region where add/remove are implemented
rg -n "addMeshFromGeometry|removeMeshById|computeBvhAsync" "$FILE_CONTROLLER"
sed -n '120,260p' "$FILE_CONTROLLER" | awk '{print NR "\t" $0}'
echo "---- precomputed_mesh_saga.ts imports for mergeGeometries/computeBvhAsync"
sed -n '1,120p' "$FILE_SAGA" | awk '{print NR "\t" $0}'
echo "---- find mergeGeometries import source and any local helpers"
rg -n "mergeGeometries" "$FILE_SAGA" "$FILE_CONTROLLER" "$FILE_HELPERS"Repository: scalableminds/webknossos
Length of output: 16273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_CONTROLLER="frontend/javascripts/viewer/controller/segment_mesh_controller.ts"
FILE_SAGA="frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts"
echo "---- occurrences of dispose() in controller"
rg -n "dispose\(" "$FILE_CONTROLLER"
echo "---- implementation of removeMeshById and any geometry disposal"
# print around removeMeshById function
rg -n "removeMeshById\(" "$FILE_CONTROLLER"
# rough region around it
sed -n '260,360p' "$FILE_CONTROLLER" | awk '{print NR+259 "\t" $0}'
echo "---- any disposal/cleanup for mergedGeometry in saga"
rg -n "mergedGeometry\.dispose|dispose\(" "$FILE_SAGA"
rg -n "Falling back to the unmerged mesh chunks|mergedGeometry == null" "$FILE_SAGA"
sed -n '480,540p' "$FILE_SAGA" | awk '{print NR+479 "\t" $0}'Repository: scalableminds/webknossos
Length of output: 4519
Dispose merged geometry when merge/BVH fails
In the catch block, mergedGeometry is set to null without calling dispose(). On this path the merged geometry was never handed to segmentMeshController/scene management (and removeMeshById/disposal isn’t invoked because the saga returns immediately), so the allocated Three.js BufferGeometry from mergeGeometries can leak GPU resources when computeBvhAsync throws.
Proposed fix
let mergedGeometry: BufferGeometryWithInfo | null = null;
try {
mergedGeometry = (
sortedBufferGeometries.length > 0 ? mergeGeometries(sortedBufferGeometries, false) : null
) as BufferGeometryWithInfo | null;
if (mergedGeometry != null) {
mergedGeometry.vertexSegmentMapping = new VertexSegmentMapping(sortedBufferGeometries);
mergedGeometry.boundsTree = yield* call(computeBvhAsync, mergedGeometry);
}
} catch (exception) {
+ mergedGeometry?.dispose();
mergedGeometry = null;
console.error(`Failed to merge mesh chunks for segment ${segmentId}:`, exception);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/javascripts/viewer/model/sagas/meshes/precomputed_mesh_saga.ts`
around lines 502 - 513, The catch block currently sets mergedGeometry = null
without disposing the BufferGeometry returned by mergeGeometries, causing a GPU
leak when computeBvhAsync throws; update the catch to check if mergedGeometry is
non-null and call mergedGeometry.dispose() (and also dispose
mergedGeometry.boundsTree if applicable) before setting mergedGeometry = null so
the merged BufferGeometry and any associated BVH resources are released;
reference the mergedGeometry variable and the functions mergeGeometries and
computeBvhAsync to locate the code to update.
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.
When meshes are loaded for many segments at once (e.g., for a whole segment group), large meshes could exhaust RAM, fail ungracefully during geometry merging, spam error toasts on flaky chunk requests, and stall the UI. This PR addresses the four smaller issues from that problem set (memory accounting/LOD eviction is left for a follow-up):
SegmentMeshController.removeMeshByIdnow disposes the geometries/materials of removed meshes so that three.js actually frees the GPU buffers. This also covers the eagerly added chunk meshes that are replaced by the merged geometry after every precomputed mesh load, as well as mesh reloads and removals.PARALLEL_MESH_LOADING_SEGMENT_COUNT = 4). Loading indicators still appear immediately for all queued segments, but the download/decode/merge pipelines run for at most 4 segments at a time. This way, the first meshes are fully visible earlier and the memory pressure of in-flight buffers stays bounded. Cancellation via REMOVE_MESH keeps working for both queued and running loads.mergeGeometries+ BVH computation are now wrapped in try/catch. If merging fails (e.g., buffer allocation failure under memory pressure), the eagerly rendered, unmerged chunk meshes are kept as a fallback instead of the error killing the saga. Only features that need the merged geometry (proofreading range highlighting, fast raycasting) are unavailable for that mesh then.Steps to test:
renderer.info.memory.geometries) that memory is released.TODOs:
Issues:
fixes #9541
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)🤖 Generated with Claude Code