labels rendering performance improvement: create ImageBitmaps in worker #5169
Conversation
WalkthroughThe changes in this pull request involve significant refactoring of the Changes
Possibly related PRs
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
force-pushed
the
improv/image-bitmaps
branch
2 times, most recently
from
69d495a
to
a782eff
Compare
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
a782eff
to
a38a8b0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (18)
app/packages/looker/src/worker/shared.ts (2)
Line range hint
6-12: Add TypeScript type annotations for better type safety.The function lacks TypeScript type annotations which could lead to type-safety issues.
Consider applying this improvement:
-export const mapId = (obj) => { +export const mapId = <T extends { _id?: any; id?: any }>(obj: T): T => { if (obj && obj._id !== undefined) { obj.id = obj._id; delete obj._id; } return obj; };
14-21: Add JSDoc documentation and improve type safety.While this function is part of the labels rendering performance improvement, it could benefit from better documentation and type safety.
Consider these improvements:
+/** + * Returns the overlay field configuration based on the class type. + * Used in the worker thread for efficient overlay data processing. + * @param cls - The class type of the overlay + * @returns Object containing canonical and disk field names + */ -export const getOverlayFieldFromCls = (cls: string) => { +type OverlayField = { + canonical: "map" | "mask"; + disk: "map_path" | "mask_path"; +}; +export const getOverlayFieldFromCls = (cls: string): OverlayField => { switch (cls) { case HEATMAP: return { canonical: "map", disk: "map_path" }; default: return { canonical: "mask", disk: "mask_path" }; } };app/packages/looker/src/worker/pooled-fetch.ts (1)
3-4: Consider making MAX_CONCURRENT_REQUESTS configurableThe constant value of 100 is noted as arbitrary. Consider making this configurable through environment variables or configuration files to allow tuning based on system capabilities and requirements.
-// note: arbitrary number that seems to work well -const MAX_CONCURRENT_REQUESTS = 100; +// Maximum number of concurrent requests allowed +const MAX_CONCURRENT_REQUESTS = process.env.MAX_CONCURRENT_REQUESTS + ? parseInt(process.env.MAX_CONCURRENT_REQUESTS, 10) + : 100;app/packages/looker/src/worker/decorated-fetch.ts (1)
Line range hint
26-28: Enhance error messages with request detailsConsider adding more context to error messages to aid debugging:
- throw new NonRetryableError(`Non-retryable HTTP error: ${response.status}`); + throw new NonRetryableError(`Non-retryable HTTP error ${response.status} for URL: ${url}`); - throw new Error( - "Max retries for fetch reached (linear backoff), error: " + e - ); + throw new Error( + `Max retries (${retries}) reached for URL: ${url}, last error: ${e}` + );Also applies to: 41-44
app/packages/looker/src/worker/decorated-fetch.test.ts (3)
18-18: Consider adding type for the empty options objectWhile the empty object works, explicitly typing it as
RequestInitwould improve type safety and documentation.-expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai", {}); +expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai", {} as RequestInit);
49-50: Consider testing with specific optionsThe test only verifies behavior with empty options. Consider adding test cases with actual fetch options to ensure they're properly passed through.
// Example additional test case: it("should properly pass through fetch options", async () => { const mockResponse = new Response("Success", { status: 200 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); const options: RequestInit = { headers: { 'Content-Type': 'application/json' }, method: 'POST' }; await fetchWithLinearBackoff("http://fiftyone.ai", options); expect(global.fetch).toHaveBeenCalledWith( "http://fiftyone.ai", expect.objectContaining(options) ); });
76-81: Consider improving test readabilityThe multi-line function call could be more concise while maintaining readability.
- const fetchPromise = fetchWithLinearBackoff( - "http://fiftyone.ai", - {}, - 5, - 10 - ); + const fetchPromise = fetchWithLinearBackoff("http://fiftyone.ai", {}, 5, 10);app/packages/looker/src/overlays/base.ts (1)
43-46: Well-structured type definition supporting the performance improvement initiative.The
LabelMasktype elegantly supports the transition from raw image buffers toImageBitmap, making both properties optional for backward compatibility during the migration.Consider adding JSDoc comments to document:
- The purpose of this type in the context of worker-based image processing
- The relationship between
bitmapanddataproperties- When to expect each property to be present/absent
app/packages/looker/src/overlays/segmentation.ts (1)
80-87: Consider extracting alpha handling to a reusable function.The alpha handling logic could be extracted to a reusable function to maintain consistency across different overlay types.
+const drawWithAlpha = ( + ctx: CanvasRenderingContext2D, + alpha: number, + drawFn: () => void +) => { + const tmp = ctx.globalAlpha; + ctx.globalAlpha = alpha; + drawFn(); + ctx.globalAlpha = tmp; +}; if (this.label.mask?.bitmap) { const [tlx, tly] = t(state, 0, 0); const [brx, bry] = t(state, 1, 1); - const tmp = ctx.globalAlpha; - ctx.globalAlpha = state.options.alpha; - ctx.drawImage(this.label.mask.bitmap, tlx, tly, brx - tlx, bry - tly); - ctx.globalAlpha = tmp; + drawWithAlpha(ctx, state.options.alpha, () => { + ctx.drawImage(this.label.mask.bitmap, tlx, tly, brx - tlx, bry - tly); + }); }app/packages/looker/src/worker/disk-overlay-decoder.test.ts (3)
15-19: Remove unused mock constant.The
DETECTIONconstant is mocked but never used in the test suite. Consider removing it to maintain clean and minimal mocks.vi.mock("@fiftyone/utilities", () => ({ - DETECTION: "Detection", DETECTIONS: "Detections", HEATMAP: "Heatmap", }));
36-216: Consider adding more test cases for comprehensive coverage.The test suite is well-structured and covers the main scenarios. However, consider adding the following test cases to improve coverage:
- Test concurrent processing by verifying the behavior when multiple
maskPathDecodingPromisesare processed simultaneously- Test memory cleanup by verifying that resources (e.g., ArrayBuffers) are properly disposed
- Add edge cases for shape validation (e.g., empty shape, invalid dimensions)
Here's a sample test case for concurrent processing:
it("should handle concurrent processing of multiple overlays", async () => { const maskPathDecodingPromises: Promise<void>[] = []; const labels = Array(3).fill(null).map((_, i) => ({ mask_path: `/path/to/mask${i}`, mask: null as MaskUnion })); // Setup mocks for concurrent requests vi.mocked(getSampleSrc).mockImplementation(path => `http://example.com${path}`); vi.mocked(fetchWithLinearBackoff).mockImplementation(() => Promise.resolve({ blob: () => Promise.resolve(new Blob(['mock data'])) } as Response) ); // Process labels concurrently await Promise.all(labels.map(label => decodeOverlayOnDisk( 'field', label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, 'Segmentation', maskPathDecodingPromises ) )); await Promise.all(maskPathDecodingPromises); // Verify all labels were processed labels.forEach(label => { expect(label.mask).toBeDefined(); expect(label.mask.image).toBeInstanceOf(ArrayBuffer); }); });
187-215: Enhance error handling test coverage.The error handling test case could be more comprehensive. Consider:
- Testing specific error types (network errors, timeout errors, etc.)
- Validating error messages
- Testing retry behavior
Here's an example enhancement:
it("should handle different types of fetch errors", async () => { const label = { mask_path: "/path/to/mask", mask: null as MaskUnion }; const maskPathDecodingPromises: Promise<void>[] = []; // Test network error vi.mocked(fetchWithLinearBackoff).mockRejectedValueOnce( new TypeError("Failed to fetch") ); await decodeOverlayOnDisk( "field", label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, "Segmentation", maskPathDecodingPromises ); expect(label.mask).toBeNull(); // Test timeout error vi.mocked(fetchWithLinearBackoff).mockRejectedValueOnce( new Error("Timeout") ); await decodeOverlayOnDisk( "field", label, COLORING, CUSTOMIZE_COLOR_SETTING, COLOR_SCALE, SOURCES, "Segmentation", maskPathDecodingPromises ); expect(label.mask).toBeNull(); // Verify retry behavior expect(fetchWithLinearBackoff).toHaveBeenCalledTimes(2); });app/packages/looker/src/lookers/abstract.ts (1)
Line range hint
601-644: Add timeout and error handling for worker communicationThe worker communication lacks timeout handling and error recovery mechanisms. Long-running or failed worker operations could leave the system in an inconsistent state.
Consider implementing timeout and error handling:
private loadSample(sample: Sample) { const messageUUID = uuid(); + const timeout = setTimeout(() => { + labelsWorker.removeEventListener("message", listener); + this.updater({ + error: new Error("Worker timeout: Failed to process sample"), + disabled: true, + reloading: false + }); + }, 30000); // 30 second timeout const labelsWorker = getLabelsWorker(); const listener = ({ data: { sample, coloring, uuid } }) => { if (uuid === messageUUID) { + clearTimeout(timeout); this.sample = sample; this.state.options.coloring = coloring; this.loadOverlays(sample); this.updater({ overlaysPrepared: true, disabled: false, reloading: false, }); labelsWorker.removeEventListener("message", listener); } }; + labelsWorker.addEventListener("error", (error) => { + clearTimeout(timeout); + labelsWorker.removeEventListener("message", listener); + this.updater({ + error: new Error(`Worker error: ${error.message}`), + disabled: true, + reloading: false + }); + }); labelsWorker.addEventListener("message", listener);app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
21-21: Use a specific type instead of 'Record<string, any>'The
labelparameter is typed asRecord<string, any>, which is too generic. Defining a specific interface forlabelwould enhance type safety and code maintainability.app/packages/looker/src/overlays/heatmap.ts (1)
Line range hint
100-150: Add null checks to prevent potential runtime errorsIn the methods
getIndex,getMapCoordinates, andgetTarget,this.label.mapandthis.targetsmay be undefined if the constructor returns early due to a missinglabel.map. Accessing their properties without null checks could lead to runtime errors.Apply the following diffs to add null checks:
In
getIndex:private getIndex(state: Readonly<State>): number { + if (!this.label.map) { + return -1; + } const [sx, sy] = this.getMapCoordinates(state); if (sx < 0 || sy < 0) { return -1; } return this.label.map.data.shape[1] * sy + sx; }In
getMapCoordinates:private getMapCoordinates({ pixelCoordinates: [x, y], dimensions: [mw, mh], }: Readonly<State>): Coordinates { + if (!this.label.map) { + return [-1, -1]; + } const [h, w] = this.label.map.data.shape; const sx = Math.floor(x * (w / mw)); const sy = Math.floor(y * (h / mh)); return [sx, sy]; }In
getTarget:private getTarget(state: Readonly<State>): number { const index = this.getIndex(state); if (index < 0) { return null; } + if (!this.label.map || !this.targets) { + return null; + } if (this.label.map.data.channels > 1) { return this.targets[index * this.label.map.data.channels]; } return this.targets[index]; }app/packages/looker/src/worker/index.ts (3)
258-258: Consider using thedeleteoperator to remove theimagepropertyInstead of setting
label[overlayField].imagetonull, you could delete the property to potentially free up memory more effectively:- label[overlayField].image = null; + delete label[overlayField].image;This change explicitly removes the property from the object.
261-267: Simplify Promise creation incollectBitmapPromisesYou can simplify the Promise handling by directly returning the
createImageBitmappromise:-bitmapPromises.push( - new Promise((resolve) => { - createImageBitmap(imageData).then((imageBitmap) => { - label[overlayField].bitmap = imageBitmap; - resolve(imageBitmap); - }); - }) -); +bitmapPromises.push( + createImageBitmap(imageData).then((imageBitmap) => { + label[overlayField].bitmap = imageBitmap; + return imageBitmap; + }) +);This refactor reduces unnecessary nesting and makes the code more concise.
332-332: Simplify conditional check with optional chainingConsider simplifying the conditional statement using optional chaining:
-if (sample.frames && sample.frames.length) { +if (sample.frames?.length) {This makes the code cleaner and handles cases where
sample.framesisundefinedornull.🧰 Tools
🪛 Biome (1.9.4)
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/overlays/base.ts(2 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(3 hunks)app/packages/looker/src/overlays/segmentation.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.test.ts(5 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)app/packages/looker/src/worker/deserializer.ts(0 hunks)app/packages/looker/src/worker/disk-overlay-decoder.test.ts(1 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)app/packages/looker/src/worker/shared.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/packages/looker/src/worker/deserializer.ts
🧰 Additional context used
📓 Path-based instructions (12)
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.
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/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/decorated-fetch.test.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/decorated-fetch.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.test.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/worker/index.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/pooled-fetch.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/shared.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.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/index.ts
[error] 332-332: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
app/packages/looker/src/worker/shared.ts (2)
1-1: LGTM!
The named import follows TypeScript best practices and supports tree-shaking.
14-21: Verify worker thread integration.
Let's ensure this utility function is properly integrated with the worker thread's ImageBitmap creation process.
✅ Verification successful
Worker thread integration is properly implemented
The utility function getOverlayFieldFromCls is correctly integrated into the worker thread's image processing pipeline:
- Used in
disk-overlay-decoder.tsto determine overlay field names for disk-based overlays - Integrated in
index.tswhere it's used in conjunction withcreateImageBitmapto process overlay data - Properly handles both heatmap and mask cases, feeding into the ImageBitmap creation process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of getOverlayFieldFromCls in worker-related files
rg -t ts "getOverlayFieldFromCls" --glob "app/packages/looker/src/worker/**"
# Verify the relationship with ImageBitmap creation
ast-grep --pattern 'createImageBitmap($$$)'
Length of output: 834
Script:
#!/bin/bash
# Let's check the context around the usage of getOverlayFieldFromCls
rg -t ts -B 5 -A 5 "getOverlayFieldFromCls" --glob "app/packages/looker/src/worker/**"
# Also check how ImageBitmap is used in relation to overlays
rg -t ts -B 5 -A 5 "createImageBitmap" --glob "app/packages/looker/src/worker/**"
Length of output: 5998
app/packages/looker/src/worker/decorated-fetch.ts (2)
15-15: LGTM! Well-structured parameter addition
The new opts parameter is well-typed using RequestInit and maintains backward compatibility with its default empty object value.
21-21: Consider cloning the opts object for each retry attempt
The current implementation reuses the same opts object across retry attempts. This could cause issues if opts contains a request body stream, as streams can only be read once. Consider cloning the opts object for each retry attempt.
Here's a suggested improvement:
- const response = await fetch(url, opts);
+ const response = await fetch(url, { ...opts });Let's verify if the codebase uses request body streams:
app/packages/looker/src/worker/decorated-fetch.test.ts (3)
38-38: LGTM! Test case properly validates retry behavior
The test case correctly verifies the error handling with the new options parameter.
60-61: LGTM! Non-retryable error handling is correct
The test properly verifies that 4xx errors are immediately thrown without retrying.
Line range hint 1-96: Verify integration with worker thread changes
While the test changes look good, we should verify how this relates to the worker thread improvements mentioned in the PR objectives.
app/packages/looker/src/overlays/base.ts (1)
6-6: LGTM!
Clean import statement following TypeScript conventions.
app/packages/looker/src/overlays/segmentation.ts (3)
5-5: LGTM: Import changes align with new functionality.
The added imports support the new bitmap-based mask handling and size estimation features.
Also applies to: 11-11
52-52: LGTM: Constructor simplified with bitmap-based approach.
The removal of canvas and imageData initialization aligns with the performance improvement goals by using ImageBitmap.
20-20: Verify the impact of the LabelMask type change.
The change from OverlayMask to LabelMask type might affect consumers of this interface.
✅ Verification successful
Type change from OverlayMask to LabelMask appears safe
The change appears to be part of a coordinated refactoring:
LabelMaskis a shared type used across multiple overlay types (detection, heatmap, segmentation)- The actual mask data handling in worker files still uses
OverlayMaskfor the underlying implementation LabelMaskappears to be an interface type for label definitions whileOverlayMaskhandles the data implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of OverlayMask type with segmentation
rg -t typescript "OverlayMask.*segmentation"
# Search for implementations of SegmentationLabel interface
ast-grep --pattern 'interface $_ extends SegmentationLabel'
Length of output: 143
Script:
#!/bin/bash
# Search for OverlayMask usage
rg "OverlayMask"
# Search for SegmentationLabel interface implementations and usage
rg "SegmentationLabel"
# Search for LabelMask usage
rg "LabelMask"
# Get the content of the file to understand the context
cat app/packages/looker/src/overlays/segmentation.ts
Length of output: 9289
app/packages/looker/src/overlays/detection.ts (4)
9-15: LGTM: Import changes align with the new LabelMask type
The imports are well-organized and properly introduce the new LabelMask type needed for the performance improvements.
19-19: LGTM: Interface update supports performance improvements
The simplified mask property type using LabelMask aligns with the PR's objective to improve rendering performance by transitioning to ImageBitmap.
34-34: LGTM: Property addition supports new bitmap-based rendering
The imageBitmap property is properly typed and initialized, supporting the transition to bitmap-based rendering.
147-157: LGTM: Efficient bitmap-based rendering implementation
The implementation correctly uses ImageBitmap for rendering, with image smoothing disabled for better performance.
app/packages/looker/src/worker/disk-overlay-decoder.test.ts (1)
34-34: LGTM! Clear and well-defined type.
The MaskUnion type effectively combines the necessary types for testing purposes.
app/packages/looker/src/worker/disk-overlay-decoder.ts (1)
85-85: Verify the order of dimensions in 'overlayMask.shape'
Ensure that overlayMask.shape returns [height, width] as expected. If it returns [width, height], the variables overlayHeight and overlayWidth should be assigned accordingly.
app/packages/looker/src/overlays/heatmap.ts (5)
9-9: Importing 'sizeBytesEstimate' for size estimation
The import statement correctly adds sizeBytesEstimate from @fiftyone/utilities for calculating the memory footprint.
18-18: Importing 'LabelMask' for label definitions
The LabelMask is appropriately imported from ./base for use in the HeatmapOverlay class.
27-27: Updated 'map' property type to 'LabelMask'
The map property in the HeatmapLabel interface is correctly updated to use LabelMask, aligning with the refactoring to simplify mask structures across overlays.
80-85: Simplified drawing logic using 'ImageBitmap'
The draw method now efficiently utilizes this.label.map?.bitmap to render the heatmap overlay directly, enhancing rendering performance.
205-207: Added getSizeBytes method for memory footprint estimation
The getSizeBytes method correctly uses sizeBytesEstimate to provide the estimated size of the heatmap label, aiding in resource management.
app/packages/looker/src/worker/index.ts (1)
97-104: Well-documented function steps in processLabels
The comprehensive comments outlining the steps in processLabels enhance readability and understanding of the processing workflow.
sashankaryal
force-pushed
the
improv/image-bitmaps
branch
from
3165f1e
to
a56869c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/looker/src/overlays/heatmap.ts (2)
80-87: LGTM: Efficient bitmap-based drawing implementation.The implementation correctly uses
drawImagewith the bitmap, maintaining alpha transparency. Consider caching theglobalAlpharestoration using a try-finally block for better error handling.Consider this more robust implementation:
if (this.label.map?.bitmap) { const [tlx, tly] = t(state, 0, 0); const [brx, bry] = t(state, 1, 1); const tmp = ctx.globalAlpha; - ctx.globalAlpha = state.options.alpha; - ctx.drawImage(this.label.map.bitmap, tlx, tly, brx - tlx, bry - tly); - ctx.globalAlpha = tmp; + try { + ctx.globalAlpha = state.options.alpha; + ctx.drawImage(this.label.map.bitmap, tlx, tly, brx - tlx, bry - tly); + } finally { + ctx.globalAlpha = tmp; + } }
209-213: LGTM: Proper resource cleanup implementation.The cleanup method correctly releases the bitmap resource, but the optional chaining can be simplified.
Consider this more concise version:
public cleanup(): void { - if (this.label.map?.bitmap) { - this.label.map?.bitmap.close(); - } + this.label.map?.bitmap?.close(); }app/packages/looker/src/overlays/detection.ts (1)
146-156: Add error handling for bitmap accessWhile the changes improve performance by using ImageBitmap directly, consider adding error handling for potential bitmap access issues.
private drawMask(ctx: CanvasRenderingContext2D, state: Readonly<State>) { if (!this.label.mask?.bitmap) { + console.warn('Mask bitmap not available for drawing'); return; } + try { const [tlx, tly, w, h] = this.label.bounding_box; const [x, y] = t(state, tlx, tly); const tmp = ctx.globalAlpha; ctx.globalAlpha = state.options.alpha; ctx.imageSmoothingEnabled = false; ctx.drawImage( this.label.mask.bitmap, x, y, w * state.canvasBBox[2], h * state.canvasBBox[3] ); ctx.globalAlpha = tmp; + } catch (error) { + console.error('Error drawing mask bitmap:', error); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/packages/looker/src/lookers/abstract.ts(1 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/overlays/detection.ts(3 hunks)app/packages/looker/src/overlays/heatmap.ts(3 hunks)app/packages/looker/src/overlays/segmentation.ts(5 hunks)app/packages/looker/src/worker/disk-overlay-decoder.ts(1 hunks)app/packages/looker/src/worker/pooled-fetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/packages/looker/src/lookers/abstract.ts
- app/packages/looker/src/overlays/base.ts
- app/packages/looker/src/worker/disk-overlay-decoder.ts
- app/packages/looker/src/worker/pooled-fetch.ts
🧰 Additional context used
📓 Path-based instructions (3)
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/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.
🔇 Additional comments (10)
app/packages/looker/src/overlays/heatmap.ts (3)
9-9: LGTM: Import changes align with new functionality.
The new imports support the bitmap-based implementation and size estimation features.
Also applies to: 18-18
205-207: LGTM: Memory estimation method added.
The getSizeBytes implementation provides useful memory usage information.
27-27: LGTM: Interface update supports bitmap-based rendering.
The change from HeatMap to LabelMask aligns with the new bitmap-based rendering approach.
Let's verify that all usages of the old HeatMap interface have been updated:
✅ Verification successful
Interface change is safe and properly implemented
The search results confirm that:
- The old
HeatMapinterface is completely removed from the codebase LabelMaskis properly defined inbase.tsand consistently used across different overlay types (heatmap, detection, segmentation)- All relevant files have been updated to use the new
LabelMasktype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old HeatMap interface
rg "HeatMap[^L]" --type ts
Length of output: 28
Script:
#!/bin/bash
# Let's try a more comprehensive search for HeatMap
rg "HeatMap" --type ts -A 3
# Also search for any files containing both HeatMap and LabelMask
rg "LabelMask" --type ts -A 3
Length of output: 2597
app/packages/looker/src/overlays/segmentation.ts (5)
5-5: LGTM: Interface changes align with performance improvements
The simplified SegmentationLabel interface using LabelMask type supports the transition to ImageBitmap-based rendering.
Also applies to: 11-11, 20-20
52-52: LGTM: Constructor simplified for bitmap-based rendering
The removal of canvas initialization code aligns with the performance improvement goals while maintaining necessary target data handling.
260-262: Size estimation doesn't include bitmap memory
This is a known limitation where sizeBytesEstimate doesn't account for ImageBitmap memory usage.
80-87: LGTM: Efficient bitmap-based drawing implementation
The direct use of drawImage with ImageBitmap eliminates unnecessary canvas operations while maintaining proper alpha channel handling.
Let's verify the performance impact:
264-268: LGTM: Proper bitmap resource cleanup
The cleanup method correctly releases bitmap resources, which is essential for preventing memory leaks.
Let's verify cleanup is called appropriately:
app/packages/looker/src/overlays/detection.ts (2)
9-15: LGTM: Import changes align with the new ImageBitmap approach
The addition of LabelMask import supports the refactoring to use ImageBitmap for improved rendering performance.
19-19: LGTM: Interface change simplifies mask handling
The simplified mask property type aligns with the performance improvements by using LabelMask type for ImageBitmap handling.
| public cleanup(): void { | ||
| if (this.label.mask?.bitmap) { | ||
| this.label.mask?.bitmap.close(); | ||
| console.log(">>>cleanup"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug log and enhance cleanup method
The cleanup method is essential but needs improvements:
- Remove the debug console.log
- Add error handling for bitmap closure
public cleanup(): void {
if (this.label.mask?.bitmap) {
- this.label.mask?.bitmap.close();
- console.log(">>>cleanup");
+ try {
+ this.label.mask.bitmap.close();
+ } catch (error) {
+ console.error('Error closing bitmap:', error);
+ }
+ this.label.mask.bitmap = null;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public cleanup(): void { | |
| if (this.label.mask?.bitmap) { | |
| this.label.mask?.bitmap.close(); | |
| console.log(">>>cleanup"); | |
| } | |
| } | |
| public cleanup(): void { | |
| if (this.label.mask?.bitmap) { | |
| try { | |
| this.label.mask.bitmap.close(); | |
| } catch (error) { | |
| console.error('Error closing bitmap:', error); | |
| } | |
| this.label.mask.bitmap = null; | |
| } | |
| } |
💡 Codebase verification
Memory leak risk: Cleanup method not properly invoked
Based on the codebase analysis, there are potential memory leak concerns:
- The
cleanup()method is defined in multiple overlay classes (Detection, Segmentation, Heatmap) but there's no clear evidence of it being called during overlay disposal - While overlays are instantiated in
overlays/index.ts, there's no corresponding cleanup logic visible - No component unmounting or disposal logic was found that invokes these cleanup methods
Recommendations:
- Ensure
cleanup()is called when overlays are disposed - Add cleanup calls in the parent component's unmounting lifecycle
- Consider implementing a disposal pattern in the overlay management logic
🔗 Analysis chain
Verify cleanup method invocation
Ensure that the cleanup method is called appropriately when the overlay is destroyed or removed.
Let me gather more information about where these cleanup methods are being called.
Let me check one more aspect to see how these overlay classes are used and disposed.
Let me try one final search to understand the overlay lifecycle management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cleanup method calls
rg -A 5 "cleanup\(\)" app/packages/looker/src/
# Search for component unmounting or disposal logic
ast-grep --pattern 'class $_ extends $_ {
$$$
componentWillUnmount() {
$$$
}
$$$
}'
Length of output: 1354
Script:
#!/bin/bash
# Search for imports and usage of these overlay classes
rg -A 2 "import.*\{.*(Detection|Segmentation|Heatmap).*\}" app/packages/looker/src/
# Search for instances where these overlays are destroyed/disposed
rg -B 2 -A 2 "dispose\(\)|destroy\(\)" app/packages/looker/src/
# Look for any class that might be managing these overlays
ast-grep --pattern 'class $_ {
$$$
dispose() {
$$$
}
$$$
}'
Length of output: 3128
Script:
#!/bin/bash
# Search for overlay usage in components and their lifecycle methods
rg -B 3 -A 3 "new (Detection|Segmentation|Heatmap)Overlay" app/packages/looker/src/
# Look for any cleanup calls in component unmounting
ast-grep --pattern 'componentWillUnmount() {
$$$
}'
# Check if overlays are managed in any manager class
rg -B 2 -A 5 "overlay" app/packages/looker/src/managers/
Length of output: 272
Script:
#!/bin/bash
# Search for overlay instantiation and management
rg -B 3 -A 3 "new.*Overlay" app/packages/looker/src/
# Look for cleanup or disposal patterns in the main components
rg -B 3 -A 3 "\.cleanup\(\)" app/packages/looker/src/
# Check base overlay class for lifecycle management
ast-grep --pattern 'class $_ {
$$$
cleanup() {
$$$
}
$$$
}'
Length of output: 723
What changes are proposed in this pull request?
Continues efforts in #5156 to improve FiftyOne's overlays rendering performance.
Instead of transferring raw 32 bit image array buffers from worker to the main thread, we create ImageBitmap in the worker and transfer that instead.
Before
Worker
Main thread
Overlay constructor
ImageDatato the ephemeral canvasdrawMaskNow
Worker
Main thread
Overlay constructor
drawMaskImageBitmapfrom worker to main canvasHow is this patch tested? If it is not, please explain why.
Locally. Todo: more rigorous performance testing
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation
fetchWithLinearBackofffunction to ensure consistent usage of options.Refactor
processLabelsandprocessSamplefunctions to accommodate new processing logic.