improve overlay rendering performance #5156
Conversation
WalkthroughThe changes introduced in this pull request enhance the error handling and retry mechanisms for image loading and HTTP fetch operations within the Looker application. The Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (12)
app/packages/looker/src/worker/decorated-fetch.ts (2)
1-8: Add JSDoc documentation for the function and its parametersThe function signature is well-defined, but adding JSDoc documentation would improve maintainability and developer experience.
Add documentation like this:
+/** + * Fetches a resource with linear backoff retry strategy + * @param url - The URL to fetch + * @param retries - Maximum number of retry attempts (default: 10) + * @param delay - Base delay in milliseconds between retries (default: 200) + * @returns Promise<Response> - The fetch response + * @throws Error if max retries reached or HTTP error occurs + */ export const fetchWithLinearBackoff = async (
1-25: Consider implementing adaptive retry strategiesWhile linear backoff is an improvement, consider these architectural enhancements:
- Different retry strategies (exponential, jittered) for different types of failures
- Resource-specific retry configurations (fewer retries for small resources)
- Circuit breaker pattern for system-wide issues
Would you like me to provide example implementations of these patterns?
app/packages/looker/src/elements/image.ts (3)
9-16: LGTM! Consider adding documentation for the constant.The class properties and constant are well-defined. Consider adding a JSDoc comment for
MAX_IMAGE_LOAD_RETRIESto explain the rationale behind the value of 10 attempts.+/** + * Maximum number of retry attempts for image loading. + * Set to 10 to handle temporary resource insufficiency while preventing infinite retries. + */ const MAX_IMAGE_LOAD_RETRIES = 10;
44-45: Simplify retry source URL assignment.The string template literal is unnecessary since
this.srcis already a string.-const retrySrc = `${this.src}`; -this.element.setAttribute("src", retrySrc); +this.element.setAttribute("src", this.src);
66-70: Refactor duplicate cleanup logic.The timeout cleanup logic is duplicated across methods. Consider extracting it to a reusable private method.
+private cleanup() { + this.retryCount = 0; + if (this.timeoutId !== null) { + window.clearTimeout(this.timeoutId); + this.timeoutId = null; + } +} renderSelf({ config: { src } }: Readonly<ImageState>) { if (this.src !== src) { this.src = src; - this.retryCount = 0; - if (this.timeoutId !== null) { - window.clearTimeout(this.timeoutId); - this.timeoutId = null; - } + this.cleanup(); this.element.setAttribute("src", src); } return null; }app/packages/looker/src/worker/decorated-fetch.test.ts (4)
10-19: Consider adding response body verification.While the test covers the basic success case well, it would be more robust to also verify the response body content using
response.text()orresponse.json().it("should return response when fetch succeeds on first try", async () => { const mockResponse = new Response("Success", { status: 200 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); const response = await fetchWithLinearBackoff("http://fiftyone.ai"); expect(response).toBe(mockResponse); + expect(await response.text()).toBe("Success"); expect(global.fetch).toHaveBeenCalledTimes(1); expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai"); });
21-32: Consider using a specific error type for network failures.To better match real-world scenarios, consider using specific error types like
TypeErrorfor network failures instead of a generic Error.global.fetch = vi .fn() - .mockRejectedValueOnce(new Error("Network Error")) + .mockRejectedValueOnce(new TypeError("Failed to fetch")) .mockResolvedValue(mockResponse);
34-42: Consider verifying the complete error chain.The test could be more thorough by verifying both the thrown error message and the underlying network error.
await expect( fetchWithLinearBackoff("http://fiftyone.ai", 3) -).rejects.toThrow("Max retries for fetch reached"); +).rejects.toThrowError(expect.objectContaining({ + message: "Max retries for fetch reached", + cause: expect.any(Error) +}));
55-79: Fix typo and enhance backoff timing verification.
- There's a typo in the comment "scond" should be "second"
- The test could be more explicit about the linear backoff calculation
// advance timers to simulate delays // after first delay await vi.advanceTimersByTimeAsync(100); - // after scond delay + // after second delay (2 * 100ms) await vi.advanceTimersByTimeAsync(200);Consider adding assertions to verify the exact timing of each retry attempt:
expect(vi.getTimerCount()).toBe(0); // Verify all timers were processed expect(vi.getRealSystemTime()).toBe(300); // Verify total delay (100ms + 200ms)app/packages/utilities/src/fetch.ts (1)
113-113: Improved retry mechanism with linear backoffAdding a 500ms delay between retries is a good improvement that:
- Prevents overwhelming the server with immediate retry attempts
- Aligns with the PR's goal of implementing linear backoff
- Helps mitigate
ERR:INSUFFICIENT_RESOURCESerrors by spacing out requestsHowever, consider making this delay configurable through a parameter to allow adjustment based on different use cases.
- retryDelay: 500, + retryDelay: options?.retryDelay ?? 500,app/packages/looker/src/worker/index.ts (2)
Line range hint
116-131: Pass 'maskPathDecodingPromises' to recursive 'imputeOverlayFromPath' callsIn the recursive call to
imputeOverlayFromPathfor detections, themaskPathDecodingPromisesparameter is not passed. This may result inmaskPathDecodingPromisesbeing undefined within these recursive calls, causing a runtime error when attempting to push into it.Apply this diff to pass
maskPathDecodingPromisesto the recursive call:for (const detection of label.detections) { promises.push( imputeOverlayFromPath( field, detection, coloring, customizeColorSetting, colorscale, buffers, {}, DETECTION, + maskPathDecodingPromises ) ); }
164-166: Consider implementing a consistent error handling strategyUsing
console.error(e);directly may not be the most effective way to handle errors, especially in production code. Consider implementing a centralized error handling or logging mechanism to improve maintainability and facilitate debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/looker/src/elements/image.ts(3 hunks)app/packages/looker/src/worker/decorated-fetch.test.ts(1 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)app/packages/utilities/src/fetch.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/looker/src/elements/image.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/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/utilities/src/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.
🔇 Additional comments (5)
app/packages/looker/src/elements/image.ts (1)
37-48: Verify performance impact of retry mechanism.
The linear backoff implementation aligns with the PR's goal of handling ERR:INSUFFICIENT_RESOURCES, but we should verify its effectiveness.
✅ Verification successful
Based on the search results, I can now provide a final response:
Linear backoff retry mechanism is properly implemented with cleanup
The implementation shows:
- Proper cleanup of timeouts via
clearTimeout(this.timeoutId) - Consistent with other retry mechanisms in the codebase (e.g.
decorated-fetch.ts) - Linear backoff delay using
1000 * this.retryCountmatches the documented pattern - Memory leaks are prevented through timeout cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any similar retry mechanisms in the codebase to ensure consistency
rg -g '*.{ts,tsx}' 'setTimeout.*retry|backoff'
# Look for potential memory leak patterns
ast-grep --pattern 'setTimeout($$$)'
# Check for proper cleanup patterns
ast-grep --pattern 'clearTimeout($$$)'
Length of output: 146899
app/packages/looker/src/worker/decorated-fetch.test.ts (2)
1-9: LGTM! Clean test setup with proper mock handling.
The test setup follows best practices by clearing mocks and resetting timers before each test.
44-53: Verify if 5 retries is the expected behavior for HTTP errors.
The test shows that the function retries 5 times on a 404 error. This might not be the optimal behavior since 404 is a definitive error that typically shouldn't trigger retries.
app/packages/utilities/src/fetch.ts (2)
21-21: Type simplification improves interface clarity
Removing the "arrayBuffer" string literal type from retryCodes parameter makes the interface more focused and type-safe. The parameter now consistently accepts only an array of HTTP status codes for retry logic.
Line range hint 113-120: Verify retry behavior under high load
The retry mechanism is crucial for handling overlay fetching. Let's verify its behavior under load conditions.
✅ Verification successful
Retry mechanism implementation is robust and well-tested
The verification reveals a well-structured retry mechanism with:
- Default retry delay of 500ms in the utilities package
- Proper error handling for insufficient resources in the image loading component
- Comprehensive test coverage including linear backoff testing
- Multiple implementations tailored for different use cases (fetch-retry in utilities and custom implementation in looker package)
Key findings:
- The retry configuration in
app/packages/utilities/src/fetch.tsis aligned with other implementations - Tests in
app/packages/looker/src/worker/decorated-fetch.test.tsverify behavior under load conditions - Image loading component specifically handles insufficient resources errors with retries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for retry-related error patterns and configurations across the codebase
# Check for other retry delay configurations
echo "Searching for retry delay configurations:"
rg -i "retryDelay|retry.*delay" --type ts
# Check for ERR:INSUFFICIENT_RESOURCES error handling
echo "Searching for insufficient resources error handling:"
rg -i "ERR:INSUFFICIENT_RESOURCES|insufficient.*resources" --type ts
# Look for related error handling patterns
echo "Searching for related fetch error handling:"
rg "fetchRetry|retryOn|retries" --type ts -A 3
Length of output: 5479
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/packages/looker/src/worker/decorated-fetch.test.ts (4)
10-19: Consider using more realistic test data.While the test is well-structured, consider using a more realistic JSON response body that matches the actual API response structure. This would make the tests more valuable for catching serialization issues.
- const mockResponse = new Response("Success", { status: 200 }); + const mockResponse = new Response(JSON.stringify({ + status: "success", + data: { /* add expected data structure */ } + }), { + status: 200, + headers: { 'Content-Type': 'application/json' } + });
44-53: Consider testing multiple HTTP error status codes.The test could be more comprehensive by using test.each to verify handling of different HTTP status codes (e.g., 400, 401, 403, 500).
it.each([ [400, 'Bad Request'], [401, 'Unauthorized'], [403, 'Forbidden'], [500, 'Internal Server Error'] ])('should throw an error for HTTP %i status', async (status, statusText) => { const mockResponse = new Response(statusText, { status }); global.fetch = vi.fn().mockResolvedValue(mockResponse); await expect( fetchWithLinearBackoff("http://fiftyone.ai", 5, 10) ).rejects.toThrow(`HTTP error: ${status}`); });
70-70: Fix typo in comment.There's a typo in the comment: "scond" should be "second".
- // after scond delay + // after second delay
1-80: Well-structured test suite that aligns with PR objectives.The test suite provides comprehensive coverage of the new
fetchWithLinearBackofffunction, which is crucial for improving overlay rendering performance. The tests effectively validate:
- Basic success scenarios
- Retry mechanism
- Error handling
- Backoff timing
This robust testing will help ensure reliability when loading overlays, particularly in scenarios that might trigger
ERR:INSUFFICIENT_RESOURCES.🧰 Tools
🪛 Biome
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/packages/looker/src/worker/index.ts (2)
159-167: Enhance error logging for better debuggingWhile the error handling is good, consider adding more context to the error log.
- console.error(e); + console.error(`Failed to fetch overlay image from ${baseUrl}:`, e);
195-196: Consider clearing promise arrays after useTo optimize memory usage, consider clearing the arrays after they're no longer needed.
return Promise.all(painterPromises).then(() => buffers); + painterPromises.length = 0; // Clear the arrayAlso applies to: 289-289
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/looker/src/worker/decorated-fetch.test.ts(1 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)app/packages/looker/src/worker/index.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/worker/decorated-fetch.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/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.
🪛 Biome
app/packages/looker/src/worker/decorated-fetch.test.ts
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (6)
app/packages/looker/src/worker/decorated-fetch.test.ts (3)
1-8: LGTM! Clean test setup with proper mock cleanup.
The test setup follows best practices by clearing mocks and resetting timers before each test.
21-32: LGTM! Good coverage of retry scenario.
The test effectively verifies that the fetch operation retries after a failure and succeeds eventually.
55-79: LGTM! Comprehensive timing verification.
The test effectively verifies the linear backoff behavior by:
- Simulating network failures
- Advancing timers to simulate delays
- Verifying the correct number of retry attempts
app/packages/looker/src/worker/index.ts (3)
33-33: LGTM: Import of robust fetch function
The addition of fetchWithLinearBackoff aligns with the PR objective of implementing a more resilient fetch mechanism.
Line range hint 111-131: LGTM: Improved promise handling for detections
The changes effectively accumulate promises instead of awaiting them immediately, which aligns with the PR objective of preventing blocking in loops during mask decoding.
Line range hint 199-259: LGTM: Effective separation of decoding and painting loops
The separation of mask decoding and painting loops, along with the use of Promise.allSettled, effectively prevents blocking during deserialization as intended in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/looker/src/elements/common/error.ts (2)
106-109: LGTM! Consider adding error handling for DOM operations.The cleanup logic is well-placed and helps prevent memory leaks. However, consider wrapping the DOM operations in a try-catch block to handle potential errors gracefully.
if (!error && this.errorElement) { - this.errorElement.remove(); - this.errorElement = null; + try { + this.errorElement.remove(); + this.errorElement = null; + } catch (e) { + console.warn('Failed to cleanup error element:', e); + this.errorElement = null; + } }
106-109: Consider deferring DOM operations for better performance.To avoid blocking the main thread during cleanup, consider deferring the DOM operation using
requestAnimationFrame.if (!error && this.errorElement) { - this.errorElement.remove(); - this.errorElement = null; + requestAnimationFrame(() => { + if (this.errorElement) { + this.errorElement.remove(); + this.errorElement = null; + } + }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/elements/common/error.ts(1 hunks)app/packages/looker/src/elements/image.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/elements/image.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/looker/src/elements/common/error.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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/packages/looker/src/worker/decorated-fetch.ts (1)
6-11: Enhance error class implementation with TypeScript best practicesThe error class implementation could be improved for better TypeScript support and error handling.
Consider applying these improvements:
class NonRetryableError extends Error { constructor(message: string) { super(message); + // Restore prototype chain + Object.setPrototypeOf(this, new.target.prototype); this.name = "NonRetryableError"; } }app/packages/looker/src/worker/decorated-fetch.test.ts (3)
10-19: Consider extracting test constants.The test is well-structured, but the URL "http://fiftyone.ai" is duplicated across multiple tests. Consider extracting it as a constant at the top of the describe block for better maintainability.
describe("fetchWithLinearBackoff", () => { + const TEST_URL = "http://fiftyone.ai"; + beforeEach(() => { vi.clearAllMocks(); vi.useRealTimers(); }); it("should return response when fetch succeeds on first try", async () => { const mockResponse = new Response("Success", { status: 200 }); global.fetch = vi.fn().mockResolvedValue(mockResponse); - const response = await fetchWithLinearBackoff("http://fiftyone.ai"); + const response = await fetchWithLinearBackoff(TEST_URL); expect(response).toBe(mockResponse); expect(global.fetch).toHaveBeenCalledTimes(1); - expect(global.fetch).toHaveBeenCalledWith("http://fiftyone.ai"); + expect(global.fetch).toHaveBeenCalledWith(TEST_URL); });
44-53: Consider adding more specific error assertions.While the test covers the 500 error case, consider adding assertions for the complete error message and type to ensure the error handling remains consistent.
await expect( fetchWithLinearBackoff("http://fiftyone.ai", 5, 10) - ).rejects.toThrow("HTTP error: 500"); + ).rejects.toThrow(expect.objectContaining({ + message: "HTTP error: 500", + name: "FetchError" // assuming you have a custom error type + }));
66-90: Enhance timing assertions and fix typo.The test effectively validates the linear backoff mechanism, but could be improved:
- Add assertions to verify the exact timing of retries
- Fix the typo in the comment ("scond" → "second")
// advance timers to simulate delays // after first delay await vi.advanceTimersByTimeAsync(100); - // after scond delay + // after second delay await vi.advanceTimersByTimeAsync(200); const response = await fetchPromise; expect(response).toBe(mockResponse); expect(global.fetch).toHaveBeenCalledTimes(3); + // Verify timing of retries + expect(vi.getTimerCount()).toBe(0); // ensure all timers are cleared + expect(vi.now()).toBe(300); // verify total elapsed time
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/looker/src/worker/decorated-fetch.test.ts(1 hunks)app/packages/looker/src/worker/decorated-fetch.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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.
🪛 Biome (1.9.4)
app/packages/looker/src/worker/decorated-fetch.test.ts
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (4)
app/packages/looker/src/worker/decorated-fetch.test.ts (4)
1-8: LGTM! Clean test setup following best practices.
The test setup properly imports necessary utilities and includes appropriate cleanup in the beforeEach hook.
21-32: LGTM! Good coverage of retry mechanism.
The test effectively validates the retry behavior with eventual success.
34-42: LGTM! Good validation of max retries scenario.
The test effectively verifies the behavior when maximum retries are reached.
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
55-64: LGTM! Excellent coverage of non-retryable errors.
The test properly validates that 4xx errors are not retried and throw appropriate errors.
What changes are proposed in this pull request?
Promise.allSettled()right before painting. This means we do not block in a loop for each mask to deserializefetchRetryfromgetFetchFunction()to not be reliable. We can safely not usegetFetchFunction()inimputeOverlayFromPath()since overlay URL is guaranteed to be a well-formed URL here.ERR:INSUFFICIENT_RESOURCES(both in image looker or worker). This PR adds resilience against that behavior.How is this patch tested? If it is not, please explain why.
Unit tests for new fetch function. Local performance test with the following script:
The dataset I worked with to get into
ERR:INSUFFICIENT_RESOURCESwas generated with the following script. This is a quickstart dataset with thousands of instance segmentations.NOTE: Make sure to have both your grid spacing and zoom to the smallest possible values to accommodate the largest possible amount of samples
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
Release Notes
New Features
Improvements
Tests