clean overlays in detach function and not during reconciliation #5337
Conversation
WalkthroughThis pull request focuses on improving resource management and cleanup processes across multiple components in the Looker and Spotlight packages. The changes primarily involve enhancing the destruction and cleanup methods in various classes, including Changes
Sequence DiagramsequenceDiagram
participant AbstractLooker
participant Overlay
AbstractLooker->>Overlay: destroy()
Overlay->>Overlay: cleanup()
Overlay-->>AbstractLooker: resources released
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/looker/src/overlays/segmentation.ts (1)
266-266: Enhance clarity on settingthis.targetstonull
This update properly clears the reference tothis.targets, aiding garbage collection. However, consider adding a brief JSDoc or inline comment explaining that this is an intentional release of resources.public cleanup(): void { this.label.mask?.bitmap?.close(); + // Intentionally nullifying references for garbage collection this.targets = null; }app/packages/looker/src/lookers/abstract.ts (1)
607-607: Good practice to clean up overlay resources upon destruction
Invokingcleanupon every overlay helps avoid potential memory leaks and prevents retaining references to stale objects. Ensure that all overlays, especially newly introduced or specialized ones, implement theircleanupmethod properly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(1 hunks)app/packages/looker/src/overlays/segmentation.ts(1 hunks)app/packages/looker/src/processOverlays.ts(2 hunks)app/packages/looker/src/worker/canvas-decoder.ts(2 hunks)app/packages/spotlight/src/createScrollReader.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
app/packages/looker/src/overlays/segmentation.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/spotlight/src/createScrollReader.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/overlays/heatmap.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/processOverlays.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/worker/canvas-decoder.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/lookers/abstract.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (12)
app/packages/spotlight/src/createScrollReader.ts (3)
12-12: Storing animation frame ID helps manage rendering loop
Good approach to store therequestAnimationFrameID for easier cleanup. No immediate concerns.
67-67: Assigning requestAnimationFrame to animationFrameId
This ensures that the animation frame handle is correctly captured for use in cleanup. Looks good.
77-84: Properly canceling animation frame and clearing timeout
This block effectively prevents resource leaks by canceling the animation frame and clearing the timeout. These steps complement thedestroyedflag, ensuring that the component no longer processes rendering tasks after destruction.app/packages/looker/src/processOverlays.ts (4)
7-7: Introducing HeatmapOverlay import
Adding theHeatmapOverlayimport is appropriate for handling heatmap feature logic.
8-8: Introducing SegmentationOverlay import
Adding theSegmentationOverlayimport is appropriate for efficiently managing segmentation overlays.
35-45: Improved condition for SegmentationOverlay
Skipping overlays that do not have the requiredmaskdata is consistent with defensive programming, ensuring only valid overlays proceed.
46-52: Refinement for HeatmapOverlay
Similar to the segmentation check, the earlycontinueavoids invalid heatmap overlays. This helps prevent exceptions or undefined behavior.app/packages/looker/src/worker/canvas-decoder.ts (4)
3-7: Offscreen canvas initialization
Defining a single offScreenCanvas and context at the module level improves efficiency by reusing these resources instead of allocating them repeatedly.
61-65: Dynamically setting canvas dimensions
Resizing the off-screen canvas to match the image dimensions is a more efficient approach than creating new canvases each time.
68-68: Extracting ImageData
Grabbing the image data in one step is straightforward and avoids additional overhead.
72-85: Optimizing grayscale conversion
Modifying the buffer in-place and slicing the data for grayscale improves performance by minimizing allocations. The loop logic appears correct and effectively handles skipping the G, B, and A channels.app/packages/looker/src/overlays/heatmap.ts (1)
211-211: Clearing references in cleanup
Settingthis.targetstonullsupports garbage collection. Good addition to ensure extended memory is freed when no longer needed.
sashankaryal
changed the title
| @@ -30,6 +32,25 @@ const processOverlays = <State extends BaseState>( | |||
|
|
|||
| if (!(overlay.field && overlay.field in bins)) continue; | |||
|
|
|||
| // todo: find a better approach / place for this. | |||
| // for instance, this won't work in detection overlay, where | |||
| // we might want the bounding boxes but masks might not have been loaded | |||
There was a problem hiding this comment.
I think these instances can be filtered out during overlay creation. But this is a good place for now 👍
| if (timeout) { | ||
| clearTimeout(timeout); | ||
| } | ||
|
|
||
| if (animationFrameId) { | ||
| cancelAnimationFrame(animationFrameId); | ||
| } |
There was a problem hiding this comment.
It seems like there is redundancy here. I would be surprised if the closure here is causing a memory leak. But it doesn't hurt 👍
What changes are proposed in this pull request?
detachas opposed to reconciliation functionHow is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Performance Improvements
Bug Fixes
New Features