fix(segmentation): reuse VTK actors on scroll instead of destroy/recreate#2676
Closed
pedrokohler wants to merge 2 commits into
Closed
fix(segmentation): reuse VTK actors on scroll instead of destroy/recreate#2676pedrokohler wants to merge 2 commits into
pedrokohler wants to merge 2 commits into
Conversation
Contributor
Author
|
@sedghi TL;DR this solves a very serious memory leak issue that out team here at Centaur has experienced: as you scroll through the cornerstone viewport, the allocated heap memory increases dramatically. Notice the value at the bottom right side memory_leak.1.mp4 |
sedghi
reviewed
Mar 27, 2026
|
|
||
| if (!segmentationActorInput) { | ||
| // i guess we need to create here | ||
| const reusableEntry = actors.find( |
Member
There was a problem hiding this comment.
I remember there was an actual reason for this, and i don't remember why. I let QA test it and maybe you fixed it or got fixed
Contributor
Author
|
@sedghi it's been almost 2 months since QA was asked to review. Could we merge? |
Member
Contributor
Author
|
@sedghi 😢 |
Member
|
can you rebase |
…eate Ported onto the Cornerstone3D 5.0 generic-viewport rearchitecture (cornerstonejs#2748), which moved the stack labelmap sync into syncStackLabelmapActors. Each scroll on a StackViewport with segmentations removed every labelmap actor for the previous slice and created fresh VTK objects (vtkImageData + backing TypedArray) for the new slice. vtk.js internal caches retain the discarded objects, so the heap grows linearly while scrolling (~3.9 MB per scroll with many segmentations) and is never reclaimed. For the legacy StackViewport (which renders directly from each actor's vtkImageData and does not track actors through the generic-viewport data registry), reuse the existing actors on scroll: when no actor exactly matches a slice's derived image id, swap a stale actor's underlying labelmap image and origin in place via setDerivedImage / updateVTKImageDataWithCornerstoneImage. Stale actors form a reuse pool that is consumed at most once per derived image, so overlapping segments (multiple labelmap groups on the same slice) cannot steal one another's actor. Pooled actors not reused are removed, and brand-new actors are only created when the pool is exhausted. Registry-backed viewports (PlanarViewport and its legacy adapter, detected via the presence of removeData) own their actors by dataId, so their existing remove/recreate flow is preserved to avoid desyncing that state. Also destructure callback out of addImages in StackViewport before the spread, so the callback closure (which can reference a large vtkImageData) is not persisted on the long-lived actor entry in the _actors map. Made-with: Cursor
…t prevention Unit-test the in-place actor reuse algorithm in syncStackLabelmapActors: normal single-actor scroll, distinct actor per overlapping segment group, new-actor creation only once the reuse pool is exhausted, removal of pooled actors when groups shrink, exact-match updates not consuming the pool, and a regression case showing the actor theft that per-derived-image consumption prevents. Made-with: Cursor
pedrokohler
force-pushed
the
fix/memory-leak-vtk-actor-reuse-on-scroll
branch
from
9a64cab to
9bcfb66
Compare
📝 WalkthroughWalkthroughThe PR updates ChangesStack image and labelmap actor updates
Sequence Diagram(s)sequenceDiagram
participant syncStackLabelmapActors
participant viewport
participant utilities
syncStackLabelmapActors->>viewport: removeLabelmapRepresentationData for stale actor entries
alt canReuseActorsInPlace and reusable actor entry exists
syncStackLabelmapActors->>utilities: setDerivedImage or updateVTKImageDataWithCornerstoneImage
syncStackLabelmapActors->>syncStackLabelmapActors: update referencedId and representationUID
else no reusable actor entry
syncStackLabelmapActors->>viewport: addImages for the derived image
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
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
`@packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts`:
- Around line 34-107: The spec currently reimplements the syncing logic in
simulateSync and simulateBuggyReuse instead of exercising
syncStackLabelmapActors, so regressions in the real implementation can slip
through. Rewrite these tests to call syncStackLabelmapActors directly with
mocked viewport/actor state and assertions on the resulting actor mutations,
pool cleanup, and viewport interactions, using the existing LABELMAP-related
setup and any helper mocks already present in the file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b87c03ac-3049-45da-a906-f3852b12e7c0
📒 Files selected for processing (3)
packages/core/src/RenderingEngine/StackViewport.tspackages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.tspackages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.ts
Comment on lines
+34
to
+107
| function simulateSync( | ||
| existingActors: MockActorEntry[], | ||
| derivedImageIds: string[], | ||
| segmentationId: string | ||
| ) { | ||
| const derivedImageIdSet = new Set(derivedImageIds); | ||
| const reusablePool = existingActors.filter( | ||
| (actor) => !derivedImageIdSet.has(actor.referencedId) | ||
| ); | ||
|
|
||
| const updatedInPlaceUids: string[] = []; | ||
| const reusedActorUids: string[] = []; | ||
| let createdCount = 0; | ||
|
|
||
| derivedImageIds.forEach((derivedImageId) => { | ||
| const exactMatch = existingActors.find( | ||
| (actor) => actor.referencedId === derivedImageId | ||
| ); | ||
|
|
||
| if (exactMatch) { | ||
| updatedInPlaceUids.push(exactMatch.uid); | ||
| return; | ||
| } | ||
|
|
||
| const reusable = reusablePool.shift(); | ||
|
|
||
| if (reusable) { | ||
| reusedActorUids.push(reusable.uid); | ||
| reusable.referencedId = derivedImageId; | ||
| reusable.representationUID = `${segmentationId}-${LABELMAP}-${derivedImageId}`; | ||
| return; | ||
| } | ||
|
|
||
| createdCount++; | ||
| }); | ||
|
|
||
| const removedActorUids = reusablePool.map((actor) => actor.uid); | ||
|
|
||
| return { updatedInPlaceUids, reusedActorUids, createdCount, removedActorUids }; | ||
| } | ||
|
|
||
| /** | ||
| * The pre-fix path: it picked a reusable actor from the pool without consuming | ||
| * it, so two derived images (overlapping segment groups) could resolve to the | ||
| * same actor (actor theft), leaving the other group invisible. | ||
| */ | ||
| function simulateBuggyReuse( | ||
| existingActors: MockActorEntry[], | ||
| derivedImageIds: string[] | ||
| ) { | ||
| const derivedImageIdSet = new Set(derivedImageIds); | ||
| const reusablePool = existingActors.filter( | ||
| (actor) => !derivedImageIdSet.has(actor.referencedId) | ||
| ); | ||
| const reusedActorUids: string[] = []; | ||
|
|
||
| derivedImageIds.forEach((derivedImageId) => { | ||
| const exactMatch = existingActors.find( | ||
| (actor) => actor.referencedId === derivedImageId | ||
| ); | ||
|
|
||
| if (exactMatch) { | ||
| return; | ||
| } | ||
|
|
||
| const reusable = reusablePool[0]; // BUG: pool entry is never consumed | ||
|
|
||
| if (reusable) { | ||
| reusedActorUids.push(reusable.uid); | ||
| } | ||
| }); | ||
|
|
||
| return { reusedActorUids }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Exercise syncStackLabelmapActors directly in this spec.
simulateSync and simulateBuggyReuse reimplement the intended algorithm instead of calling the production function, so this suite can stay green even if syncStackLabelmapActors regresses in actor mutation, pool cleanup, or viewport interaction. Please rewrite these as tests around the real function with mocked viewport/actor state.
🤖 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
`@packages/tools/src/tools/displayTools/Labelmap/syncStackLabelmapActors.spec.ts`
around lines 34 - 107, The spec currently reimplements the syncing logic in
simulateSync and simulateBuggyReuse instead of exercising
syncStackLabelmapActors, so regressions in the real implementation can slip
through. Rewrite these tests to call syncStackLabelmapActors directly with
mocked viewport/actor state and assertions on the resulting actor mutations,
pool cleanup, and viewport interactions, using the existing LABELMAP-related
setup and any helper mocks already present in the file.
Member
|
So are you certain we need this? it might have been fixed in the recent push for next segmentation stuff see, i can't reproduce https://cleanshot.com/share/6NnbgLly |
Contributor
Author
|
@sedghi indeed, it looks like this memory leak was fixed sometime over the past 3 months, so I’ll close this PR. Thanks for taking the initiative to review it. That said, it’s pretty discouraging to have a PR sit pending review for 3 months, only to find out after rebasing that it’s no longer relevant. Please pass that feedback along to the QA team. |
Member
|
I agree, we have a PR review speed problem we are fixing it |
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
2 participants
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.
Context
Each scroll event on a StackViewport with segmentations destroys all segmentation
actors and creates fresh VTK objects (
vtkImageData+ backingTypedArray) forevery segmentation on the new slice. With 13 segmentations at 512x512 Uint8
resolution, this allocates ~3.4 MB per scroll of new VTK objects. VTK.js internal
caches retain references to the old objects, preventing garbage collection and
causing a linear memory leak.
Reproduction:
Changes & Results
1. Reuse VTK actors on scroll (
imageChangeEventListener.ts):Instead of destroying actors on every slice change, find existing actors matching
the segmentation's representation UID prefix and update their
vtkImageDataorigin and pixel data in-place via
updateVTKImageDataWithCornerstoneImage. Userepresentation UID prefix matching for stale actor cleanup (instead of per-slice
image ID matching), so actors survive across slices.
2. Prevent actor theft for overlapping segments (
imageChangeEventListener.ts):When a segmentation has overlapping segments (multiple labelmap groups per slice),
the reusable-actor search could return the same actor for multiple groups. Track
consumed actor UIDs in a
Set<string>so each actor is reused at most once.3. Prevent callback closure leak (
StackViewport.ts):addImages()spread...restof the stack input into the actor entry stored inthe
_actorsMap. This inadvertently captured thecallbackclosure (whichholds a reference to
imageData) in the long-lived actor entry. Destructurecallbackout before the spread so it is not persisted.4. Exclude test files from ESM build (
packages/tools/tsconfig.json):Add
./src/**/__tests__/**to theexcludearray so test files using Jestmatchers don't break the
build:esmcompilation.Before: ~3.9 MB/scroll memory leak, heap grows linearly and never reclaims.
After: heap stays flat during scrolling — actors are reused in-place.
Testing
npx jest imageChangeEventListener— all pass.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests