sample update / overlay recoloring performance optimization #5247
Conversation
|
Warning Rate limit exceeded@sashankaryal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
sashankaryal
requested review from
a team
and removed request for
benjaminpkane
![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
🧹 Outside diff range and nitpick comments (5)
app/packages/looker/src/overlays/base.ts (2)
45-45: Consider documenting the newclosedBitmapDimsproperty inLabelMaskAdding the
closedBitmapDimsproperty enhances the management of bitmap dimensions. Ensure this property is well-documented and consistently used across all relevant code to prevent confusion and maintain code clarity.
87-87: Reconsider changinglabelfromprotectedtoreadonlyChanging
labelfromprotectedtoreadonlyin theCoordinateOverlayclass increases its visibility from subclasses to all external code. This may expose internal implementation details and break encapsulation. Unless there is a strong need forlabelto be publicly accessible, consider keeping itprotectedto maintain proper encapsulation.app/packages/looker/src/overlays/heatmap.ts (1)
42-42: Reconsider changinglabelfromprivatetoreadonlyChanging
labelfromprivatetoreadonlyin theHeatmapOverlayclass increases its visibility from within the class to all external code. This may expose internal data structures and compromise encapsulation. Unless necessary, consider keepinglabelprivateto maintain proper encapsulation and information hiding.app/packages/looker/src/overlays/segmentation.ts (1)
269-271: Remove redundant optional chainingThe optional chaining operator (
?.) on line 271 is redundant since we're already inside an if block that checks forthis.label.mask?.bitmap.- this.label.mask?.bitmap.close(); - this.label.mask.bitmap = null; + this.label.mask.bitmap.close(); + this.label.mask.bitmap = null;app/packages/looker/src/overlays/detection.ts (1)
269-271: Remove redundant optional chainingSimilar to segmentation.ts, the optional chaining operator is redundant here since we're inside an if block that checks for bitmap existence.
- this.label.mask?.bitmap.close(); - this.label.mask.bitmap = null; + this.label.mask.bitmap.close(); + this.label.mask.bitmap = null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
app/packages/looker/src/lookers/abstract.ts(4 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(2 hunks)app/packages/looker/src/overlays/segmentation.ts(2 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/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/looker/src/worker/disk-overlay-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/overlays/base.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/detection.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 (8)
app/packages/looker/src/lookers/abstract.ts (2)
Line range hint 723-758: Confirm that transfer parameter usage is consistent
In the updated loadSample method, ensure that all calls to postMessage correctly include the transfer parameter when transferring ArrayBuffer objects. This guarantees that the buffers are efficiently transferred without copying.
518-540:
Ensure ArrayBuffers are not accessed after being transferred to the worker
When transferring ArrayBuffer objects to a worker using postMessage with the transfer parameter, the main thread loses ownership of these buffers. Ensure that after transferring, the overlayData.data.buffer and related buffers are not accessed or used in the main thread to prevent potential errors due to detached buffers.
Run the following script to verify that overlayData.data.buffer and related buffers are not used after being transferred:
✅ Verification successful
Let me gather more information about how loadSample handles these buffers and where they might be used.
Let me check how the worker communication is implemented and where these buffers are used in the worker context.
Let me check one final thing about the worker communication and buffer transfer.
Buffer transfer implementation is correct and safe
The code correctly handles the transfer of ArrayBuffers to the worker:
- The buffers are collected into
arrayBuffersarray and immediately passed toloadSample(sample, arrayBuffers)as transfer list - The
loadSamplemethod properly transfers these buffers to the worker viapostMessage(as seen in the implementation) - After transfer, the code doesn't attempt to access the transferred buffers again
- The worker side (in deserializer.ts) correctly handles the received buffers
The cleanup of overlays before transfer and the immediate passing of buffers to the worker ensures there's no risk of accessing detached buffers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usage of `overlayData.data.buffer` after transfer.
rg 'overlayData\.data\.buffer' --context 5
Length of output: 1595
Script:
#!/bin/bash
# Search for loadSample implementation and its usage of arrayBuffers
ast-grep --pattern 'loadSample($$$) {
$$$
}'
# Also search for any direct references to these buffers
rg "arrayBuffers" -A 5
Length of output: 1233
Script:
#!/bin/bash
# Search for worker-related code and postMessage usage
rg "postMessage" -A 5
# Also check for any other references to these buffers in the worker
rg "transfer" --type typescript -A 5
Length of output: 19438
Script:
#!/bin/bash
# Search for loadSample implementation and any transfer-related code
rg -t ts "loadSample.*transfer|transferList" -A 5
# Also check the worker file for buffer handling
rg -t ts "data\.buffer" -A 5
Length of output: 4211
app/packages/looker/src/overlays/base.ts (1)
71-71:
Ensure optional label property is handled safely in Overlay interface
By adding label?: BaseLabel; to the Overlay interface and making it optional, ensure that all implementations and usages of overlay.label properly handle the possibility that label may be undefined, to avoid potential runtime errors.
Run the following script to find usages of overlay.label that may not handle undefined:
✅ Verification successful
Optional label property is safely handled in the codebase
The code in abstract.ts shows proper handling of the optional label property through type-guard checks:
- Uses
"mask" in overlay.labeland"map" in overlay.labelchecks before accessing properties - These checks implicitly ensure
labelexists before attempting to access its properties - The implementation follows TypeScript's type safety patterns for optional properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of `overlay.label` without null checks.
rg 'overlay\.label\b' --files-with-matches | xargs -I {} rg -n 'overlay\.label\b(?!\s*\?)' {}
Length of output: 344
Script:
#!/bin/bash
# First, let's find all files that contain 'overlay.label'
rg -l 'overlay\.label'
# Then, let's see the actual usage context with surrounding lines
rg -C 3 'overlay\.label'
# Additionally, let's look for BaseLabel type definition and its usage
ast-grep --pattern 'interface BaseLabel {
$$$
}'
# And find where Overlay interface is implemented
ast-grep --pattern 'class $_ implements Overlay {
$$$
}'
Length of output: 1299
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
59-63: LGTM!
The update correctly checks for closedBitmapDims before accessing its properties, ensuring safe handling of bitmap dimensions when re-initializing the mask image.
app/packages/looker/src/overlays/heatmap.ts (1)
211-218: Ensure proper cleanup and reuse of bitmap dimensions
In the cleanup method, you store the bitmap's dimensions in closedBitmapDims before closing the bitmap and setting it to null. This preserves the dimensions for future use and prevents memory leaks. The implementation appears correct and effectively manages bitmap resources.
app/packages/looker/src/overlays/segmentation.ts (2)
33-33: LGTM: Good use of readonly modifier
The change from private to public readonly maintains immutability while allowing direct access, which can improve performance by eliminating the need for getter methods.
266-273: LGTM: Good performance optimization
Storing bitmap dimensions before cleanup is a good optimization as it prevents the need to recalculate these values later.
app/packages/looker/src/overlays/detection.ts (1)
266-273: LGTM: Consistent implementation across overlay types
The bitmap dimension storage optimization is consistently implemented across overlay types, which is good for maintainability and performance.
sashankaryal
changed the title
sashankaryal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/looker/src/lookers/abstract.ts (1)
559-559: Unnecessary Use of.flat()onarrayBuffersThe
arrayBuffersvariable is already a flat array ofArrayBuffer, so calling.flat()is unnecessary.You can remove the
.flat()call to simplify the code:- this.loadSample(sample, arrayBuffers.flat()); + this.loadSample(sample, arrayBuffers);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/looker/src/lookers/abstract.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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 (2)
app/packages/looker/src/lookers/abstract.ts (2)
26-26: Import Statement Added Correctly
The import of LabelMask and CONTAINS from "../overlays/base" is appropriate and necessary for the updated functionality.
Line range hint 742-787: Robust Error Handling in Worker Communication
The try-catch block in the loadSample method effectively handles DataCloneError exceptions by retrying the postMessage without transferring buffers. This approach ensures the application remains robust and continues functioning even if a buffer is unexpectedly detached.
| // we'll transfer that to the worker instead of copying it | ||
| const arrayBuffers: ArrayBuffer[] = []; | ||
|
|
||
| for (const overlay of this.pluckedOverlays ?? []) { |
There was a problem hiding this comment.
I think sampleOverlays should be used here. pluckedOverlays contains the current frame overlays, and that is handled separately in VideoLooker.updateSample
There was a problem hiding this comment.
Also, should we consider doing this after the new overlays are returned? It seems like this leaves the current masks in an unusable state while we wait for the new ones
There was a problem hiding this comment.
Yeah... although we'd have to copy the bitmaps to the worker and I'd much rather avoid that. In my next PR, where I'll be working on async overlay loading, we'll be adding a "loading" state, maybe that'll make it better?
edit: see new commits and comment
| if (!sample) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: comment for this return
There was a problem hiding this comment.
removed it. had added it previously when debugging a race condition
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/looker/src/lookers/abstract.ts (2)
537-552: Consider enhancing detached buffer detectionThe current implementation has a potential edge case in older browsers where detached buffers might have non-zero
byteLength. Consider using a try-catch approach for more reliable detection.-if (typeof buffer.detached !== "undefined") { - if (buffer.detached) { - return; - } else { - arrayBuffers.push(buffer); - } -} else if (buffer.byteLength) { - arrayBuffers.push(buffer); -} +try { + if (buffer.byteLength === 0) { + continue; + } + arrayBuffers.push(buffer); +} catch (e) { + // Buffer is detached + return; +}
738-742: Add defensive programming measuresConsider adding null checks and error handling to prevent crashes if cleanup fails for an overlay.
protected cleanOverlays() { for (const overlay of this.sampleOverlays ?? []) { - overlay.cleanup(); + try { + overlay?.cleanup?.(); + } catch (error) { + console.warn('Failed to cleanup overlay:', error); + } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/overlays/detection.ts(1 hunks)app/packages/looker/src/overlays/heatmap.ts(2 hunks)app/packages/looker/src/overlays/segmentation.ts(2 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/packages/looker/src/overlays/heatmap.ts
- app/packages/looker/src/overlays/segmentation.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/overlays/detection.ts
🧰 Additional context used
📓 Path-based instructions (1)
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 (4)
app/packages/looker/src/lookers/abstract.ts (4)
26-26: LGTM: Import addition aligns with new type usage
The addition of LabelMask import is necessary for the type annotations in the sample update logic.
518-521: LGTM: Efficient buffer collection optimization
Collection of array buffers for transfer to worker prevents unnecessary copying of large data structures.
751-753: LGTM: Proper cleanup prevents memory leaks
Cleaning up old overlays before painting new ones effectively prevents memory leaks from dangling ImageBitmaps.
781-792: LGTM: Robust error handling for buffer transfer
The implementation gracefully handles DataCloneError by falling back to copying buffers when transfer fails.
|
@benjaminpkane In the latest commits, I made it so that we retain the bitmaps and clean them in worker callback. However, mask targets will be transferred, and so super super rare edge case where user wants to know mask target value when sample is processing (hover on segmentation overlay), they might not see anything. I think it's an acceptable compromise. I might reconsider cleaning overalys in |
sashankaryal
force-pushed
the
fix/transfer-bitmaps-back
branch
from
5eac87b to
4315fda
Compare
sashankaryal
force-pushed
the
fix/transfer-bitmaps-back
branch
from
4315fda to
e22b56e
Compare
| @@ -67,6 +67,7 @@ export interface Overlay<State extends Partial<BaseState>> { | |||
| draw(ctx: CanvasRenderingContext2D, state: State): void; | |||
| isShown(state: Readonly<State>): boolean; | |||
| field?: string; | |||
| label?: BaseLabel; | |||
There was a problem hiding this comment.
This is not true for Classifications. Is the CoordinateOverlay type sufficient for the purpose of this PR?
| label?: BaseLabel; |
| // if one of the buffers is detached and we didn't catch it | ||
| // try again without transferring the buffers (copying them) |
There was a problem hiding this comment.
Is guaranteeing no DataCloneError errors possible? How does a bitmap on the main thread get detached before being transferred?
There was a problem hiding this comment.
Oh, I see this is mentioned in AbstractLooker.loadSample
What changes are proposed in this pull request?
This PR makes sample update / overlay recoloring faster, by:
How is this patch tested? If it is not, please explain why.
Locally. I used the following code snippet:
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
New Features
Improvements
Bug Fixes