Looker caching updates #5051
Looker caching updates #5051
Conversation
|
Warning Rate limit exceeded@benjaminpkane has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 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. WalkthroughThe changes in this pull request involve multiple modifications across various files, primarily focusing on the removal of caching mechanisms and the simplification of resource management logic. Key alterations include the removal of components and properties related to caching, updates to function signatures, and the introduction of new constants for managing frame sizes. The overall structure and control flow of the affected components remain consistent, with a focus on enhancing clarity and reducing complexity. 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
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
app/packages/looker/src/constants.ts (1)
27-28: Add documentation for frame stream size limits.The new constants
MAX_FRAME_STREAM_SIZEandMAX_FRAME_STREAM_SIZE_BYTESlack documentation explaining their purpose and implications. Consider adding JSDoc comments to clarify:
- Why these specific values were chosen
- What happens when these limits are reached
- How they relate to the caching mechanism
+/** Maximum number of frames allowed in a stream cache */ export const MAX_FRAME_STREAM_SIZE = 5100; +/** Maximum size in bytes (1GB) for frame stream cache */ export const MAX_FRAME_STREAM_SIZE_BYTES = 1e9;app/packages/utilities/src/size-bytes-estimate.ts (4)
11-14: Enhance type checking in isObject functionThe current implementation might have false positives. Consider using a more precise type check.
-const isObject = (object: object) => { +const isObject = (object: unknown): object is Record<string, unknown> | unknown[] => { const oClass = objectClass(object); return oClass === "Object" || oClass === "Array"; };
16-18: Add JSDoc comments for type definitionsAdding documentation for these types would improve code maintainability and help other developers understand their purpose.
+/** Represents the basic types that can be measured for size */ type SizeTypes = null | object | string | undefined; +/** Extends SizeTypes to include arrays of measurable types */ type SizerTypes = SizeTypes | Array<SizeTypes>;
29-37: Extract magic numbers into named constantsThe byte size assumptions should be defined as named constants for better maintainability and documentation.
+const BOOLEAN_SIZE_BYTES = 8; +const NUMBER_SIZE_BYTES = 8; + if (typeof object === "boolean") { - // Assume 8 - return 8; + return BOOLEAN_SIZE_BYTES; } if (typeof object === "number") { - // Assume 8 - return 8; + return NUMBER_SIZE_BYTES; }
66-68: Add JSDoc documentation for the public APIThe exported function should have comprehensive documentation explaining its purpose, parameters, and return value.
+/** + * Estimates the size in bytes of a given object or primitive value. + * This is an approximation and may not reflect the exact memory usage. + * @param object - The value to measure + * @returns The estimated size in bytes + */ export default function sizeBytesEstimate(object: SizeTypes) { return sizer(object); }app/packages/core/src/components/Grid/recoil.ts (1)
Line range hint
5-9: Consider adding explicit type annotations for all atoms.While some atoms have type annotations (e.g.,
gridAt), others could benefit from explicit typing for better type safety and documentation.Example improvements:
-export const gridPage = atom({ +export const gridPage = atom<number>({ key: "gridPage", default: 0, }); -export const gridSpacing = atom({ +export const gridSpacing = atom<number>({ key: "gridSpacing", default: 3, effects: [/*...*/], });Also applies to: 11-14, 16-19, 21-25, 27-31, 33-37, 39-42
app/packages/core/src/components/Grid/useRefreshers.ts (1)
9-10: Consider documenting the rationale for cache size constantsThe magic numbers
510and1e9would benefit from documentation explaining:
- Why 510 items specifically (vs. the previous 512)?
- Is 1 billion bytes a reasonable upper limit for all deployment scenarios?
Consider adding comments:
+// Maximum number of lookers to keep in the grid cache const MAX_LRU_CACHE_ITEMS = 510; +// Maximum cache size in bytes (1GB) const MAX_LRU_CACHE_SIZE = 1e9;app/packages/state/src/hooks/index.ts (1)
12-12: Consider documenting the caching behavior change.Since this change is part of reintroducing size bytes caching for frame data streams, consider adding a comment above the
useCreateLookerexport to document the new caching behavior.+// Implements frame data stream caching with fixed size bytes export { default as useCreateLooker } from "./useCreateLooker";app/packages/looker/src/overlays/base.ts (1)
138-140: Consider adding size validation.While the implementation is correct, consider adding validation to ensure non-negative size values are returned.
getSizeBytes(): number { - return sizeBytesEstimate(this.label); + const size = sizeBytesEstimate(this.label); + return Math.max(0, size); }app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts (1)
30-30: Add JSDoc to document the constructor's purpose.While the simplified constructor signature is good, adding documentation would help explain the role of
storeBufferManagerand the class's caching behavior.+/** + * Manages frame samples with LRU caching using predefined size limits. + * @param storeBufferManager Manages the underlying buffer storage for frames + */ constructor(storeBufferManager: BufferManager) {app/packages/looker/src/lookers/frame-reader.ts (1)
Line range hint
1-170: Consider implementing a circuit breaker for memory pressure.While the current implementation handles frame caching well, consider adding a circuit breaker pattern to handle extreme memory pressure scenarios:
- Monitor memory usage trends
- Temporarily reduce
MAX_FRAME_STREAM_SIZEwhen under pressure- Implement back-pressure mechanisms in the worker communication
This would make the system more resilient to varying resource conditions.
app/packages/core/src/components/Grid/Grid.tsx (2)
65-65: Document memory implications of retained itemsHardcoding
retainItemstotruemeans items will always be retained in memory, regardless of the application state. While this simplifies the logic, it could impact memory usage with large datasets.Consider:
- Adding a comment explaining why items are always retained
- Monitoring memory usage with large datasets
- Adding a maximum retention limit if needed
Line range hint
1-186: Solid implementation with good React practicesThe Grid component demonstrates good React patterns:
- Proper use of hooks and memoization
- Clean effect cleanup
- Type-safe implementation
Consider extracting the resize handling logic into a custom hook for better reusability:
function useResizeObserver(id: string, onResize: (resizing: boolean) => void) { useEffect(() => { let width: number; let timeout: ReturnType<typeof setTimeout> | undefined; // ... existing resize logic ... }, [id, onResize]); }app/packages/looker/src/lookers/imavid/controller.ts (1)
Line range hint
1-300: Consider adding memory management documentation.The transition to fixed-size caching is a significant change in resource management. Consider adding documentation that explains:
- The rationale behind the fixed cache size
- Memory usage implications
- Guidelines for monitoring and troubleshooting cache-related issues
app/packages/state/src/hooks/useCreateLooker.ts (1)
Line range hint
35-282: LGTM: Well-structured hook implementation with proper TypeScript usage.The hook follows React best practices with proper cleanup, memoization, and type safety. Consider extracting the media type detection logic into a separate utility function to improve maintainability.
Consider refactoring the media type detection:
+ const determineMediaType = (filePath: string, mimeType: string | null): typeof ImageLooker | typeof VideoLooker | typeof ThreeDLooker | typeof ImaVidLooker | typeof FrameLooker => { + if (filePath.endsWith(".pcd") || filePath.endsWith(".fo3d")) { + return ThreeDLooker; + } + + if (mimeType === null) { + return ImageLooker; + } + + const isVideo = mimeType.startsWith("video/"); + if (isVideo && (isFrame || isPatch)) { + return FrameLooker; + } + if (isVideo) { + return VideoLooker; + } + if (!isVideo && shouldRenderImaVidLooker) { + return ImaVidLooker; + } + + return ImageLooker; + } const create = useRecoilCallback( ({ snapshot }) => ( { frameNumber, frameRate, sample, urls: rawUrls, symbol }, extra: Partial<Omit<Parameters<T["updateOptions"]>[0], "selected">> = {} ): T => { - let create: typeof FrameLooker | typeof ImageLooker | typeof ImaVidLooker | typeof ThreeDLooker | typeof VideoLooker = ImageLooker; - // ... media type detection logic ... + const create = determineMediaType(filePath, mimeType);app/packages/looker/src/overlays/classifications.ts (1)
337-339: Add TypeScript return type and documentationThe method implementation aligns well with the PR objectives for reintroducing size bytes caching. Consider these improvements:
+ /** + * Returns the estimated size in bytes of the labels data structure + * @returns {number} The estimated size in bytes + */ - getSizeBytes() { + getSizeBytes(): number { return sizeBytesEstimate(this.labels); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
app/packages/app/src/routing/RouterContext.ts(0 hunks)app/packages/core/src/components/Actions/Options.tsx(0 hunks)app/packages/core/src/components/Grid/Grid.tsx(2 hunks)app/packages/core/src/components/Grid/recoil.ts(1 hunks)app/packages/core/src/components/Grid/useRefreshers.ts(3 hunks)app/packages/looker/src/constants.ts(1 hunks)app/packages/looker/src/lookers/frame-reader.ts(3 hunks)app/packages/looker/src/lookers/imavid/controller.ts(1 hunks)app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts(3 hunks)app/packages/looker/src/lookers/video.ts(0 hunks)app/packages/looker/src/overlays/base.ts(3 hunks)app/packages/looker/src/overlays/classifications.ts(2 hunks)app/packages/looker/src/state.ts(0 hunks)app/packages/state/src/hooks/index.ts(1 hunks)app/packages/state/src/hooks/useCreateLooker.ts(2 hunks)app/packages/utilities/src/index.ts(1 hunks)app/packages/utilities/src/size-bytes-estimate.ts(1 hunks)
💤 Files with no reviewable changes (4)
- app/packages/app/src/routing/RouterContext.ts
- app/packages/core/src/components/Actions/Options.tsx
- app/packages/looker/src/lookers/video.ts
- app/packages/looker/src/state.ts
🧰 Additional context used
📓 Path-based instructions (13)
app/packages/core/src/components/Grid/Grid.tsx (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/core/src/components/Grid/recoil.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/core/src/components/Grid/useRefreshers.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/constants.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/frame-reader.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/imavid/controller.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/imavid/ima-vid-frame-samples.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/classifications.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/state/src/hooks/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/state/src/hooks/useCreateLooker.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/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/size-bytes-estimate.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 (21)
app/packages/looker/src/constants.ts (1)
25-25: Verify performance impact of increased CHUNK_SIZE.
Increasing CHUNK_SIZE from 20 to 30 could affect memory usage and processing performance. While larger chunks reduce iteration overhead, they increase per-chunk memory requirements.
✅ Verification successful
Let me analyze the usage of CHUNK_SIZE to understand its impact better.
Let me try a different approach to understand the frame chunk processing logic.
Based on the analysis of the code, I can now provide a final response about the CHUNK_SIZE change:
CHUNK_SIZE increase is safe and aligned with streaming implementation
The increase from 20 to 30 is reasonable as:
- It's used in frame-reader.ts for controlled frame streaming with proper buffering
- The implementation includes safeguards:
- Chunks are capped by frameCount using Math.min()
- Buffering state is properly managed
- Worker-based processing prevents UI blocking
- Memory impact is minimal as frames are processed asynchronously through a worker
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CHUNK_SIZE usage to understand impact
rg -A 5 "CHUNK_SIZE" --type ts
Length of output: 3288
Script:
#!/bin/bash
# Get more context around frame-reader.ts usage
ast-grep --pattern 'function $_($$_) {
$$$
CHUNK_SIZE
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Get more context around frame chunk processing
rg -B 5 -A 10 "requestFrameChunk" --type ts
Length of output: 4731
app/packages/utilities/src/size-bytes-estimate.ts (1)
20-64: Verify size estimation accuracy for frame data
Since this utility is crucial for frame data stream caching, we should verify its accuracy with actual frame data.
✅ Verification successful
Let me gather more information about the data structures being sized.
Let me check the structure of FrameSample and Overlay types to complete the verification.
Size estimation implementation is appropriate for its usage
The implementation correctly handles the data structures it's used with:
- Frame cache sizes are calculated based on overlays array, which contains objects implementing the Overlay interface
- Each overlay implements
getSizeBytes()which uses this utility - The implementation properly handles all data types found in these structures:
- Objects and arrays (frame.overlays)
- Strings (field names, labels)
- Numbers (coordinates, frame numbers)
- Boolean values (state flags)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns to understand typical data structures being sized
rg -A 5 "sizeBytesEstimate" app/packages/looker/src/
Length of output: 3791
Script:
#!/bin/bash
# Check the Frame and overlay types to understand the data structures
ast-grep --pattern 'interface Frame {
$$$
}'
# Check sample data structure
ast-grep --pattern 'interface $_ {
sample: $_;
$$$
}'
Length of output: 1391
Script:
#!/bin/bash
# Check FrameSample and Overlay types
ast-grep --pattern 'interface FrameSample {
$$$
}'
ast-grep --pattern 'interface Overlay<$_> {
$$$
}'
# Check actual usage of sizeBytesEstimate in frame-reader
rg -B 5 -A 5 "sizeCalculation.*sizeBytesEstimate" app/packages/looker/src/lookers/frame-reader.ts
Length of output: 1759
app/packages/core/src/components/Grid/recoil.ts (3)
1-3: LGTM: Clean imports and proper module organization.
The imports are well-organized, following best practices by separating external libraries from internal modules.
Line range hint 44-71: LGTM: Well-structured selector implementation.
The pageParameters selector follows best practices:
- Proper error handling for undefined dataset
- Clean parameter composition
- Type-safe callback implementation
Line range hint 1-92: Verify alignment with PR objectives.
The AI summary indicates the removal of caching-related code, but the PR objective mentions "reintroducing size bytes caching". This seems contradictory and needs clarification.
app/packages/core/src/components/Grid/useRefreshers.ts (1)
40-40: LGTM! Simplified dependency array
The removal of the caching dependency aligns with the PR objectives while maintaining necessary dependencies for layout resets.
app/packages/state/src/hooks/index.ts (1)
12-12: Verify the impact of removing frameCacheSize export.
While the export of useCreateLooker hook remains, the removal of frameCacheSize export might affect consumers that directly imported this value.
Let's verify if there are any direct imports of frameCacheSize that need to be updated:
✅ Verification successful
Removal of frameCacheSize is safe as part of caching updates
The git history shows that frameCacheSize was removed as part of commit 174e080 "remove caching options, update grid lru cache". This was a deliberate change where:
- The
frameCacheSizeselector and its underlyingframeCacheSizeStoreatom were completely removed - The
maxFrameStreamSizeparameter was removed fromImaVidFramesControllerinitialization - These changes align with the PR's objective of reintroducing size bytes caching for frame data streams
Since this was part of a larger caching architecture update and no other files were importing or using the frameCacheSize export, its removal is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct imports of frameCacheSize from this module
rg "import.*\{.*frameCacheSize.*\}.*from.*['\"]@state/hooks['\"]"
# Search for any other usage of frameCacheSize
rg "frameCacheSize" --type ts --type tsx
Length of output: 140
Script:
#!/bin/bash
# Search for direct imports of frameCacheSize using ripgrep
# Note: Using a more generic pattern and then filtering
rg "import.*frameCacheSize" -g "*.{ts,js,tsx,jsx}"
# Search for any usage of frameCacheSize
rg "frameCacheSize" -g "*.{ts,js,tsx,jsx}"
# Look for the history of the file to understand the removal
git log -p app/packages/state/src/hooks/index.ts
Length of output: 73278
Script:
#!/bin/bash
# Search for frameCacheSize in useCreateLooker.ts to understand its usage
ast-grep --pattern 'frameCacheSize' app/packages/state/src/hooks/useCreateLooker.ts
# Search for any imports of frameCacheSize in the codebase
rg "import.*frameCacheSize" -g "*.{ts,js,tsx,jsx}"
# Search for direct usage of frameCacheSize
rg "frameCacheSize" -g "*.{ts,js,tsx,jsx}"
Length of output: 179
Script:
#!/bin/bash
# Check if useCreateLooker.ts exists and examine its content
fd "useCreateLooker.ts" --exec cat {}
# Look for any references to frameCacheSize in the git history of useCreateLooker.ts
git log -p app/packages/state/src/hooks/useCreateLooker.ts | grep -A 10 -B 10 "frameCacheSize"
Length of output: 13293
app/packages/looker/src/overlays/base.ts (2)
5-6: LGTM! Clean import modifications.
The addition of sizeBytesEstimate and explicit type imports improves type safety and supports the caching functionality.
68-69: LGTM! Interface extension for size estimation.
The addition of getSizeBytes method to the Overlay interface properly supports the reintroduction of size bytes caching.
app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts (2)
6-6: LGTM: Import changes align with caching requirements.
The addition of sizeBytesEstimate and frame stream size constants supports the reintroduction of size bytes caching as intended.
Also applies to: 8-11
47-50: Consider validating size limits and documenting cache configuration.
While the static configuration is good, consider:
- Adding runtime validation to ensure
MAX_FRAME_STREAM_SIZE_BYTESis sufficient for the maximum number of frames - Documenting the cache configuration parameters for future maintenance
Let's verify the constants are properly defined:
app/packages/looker/src/lookers/frame-reader.ts (3)
1-13: LGTM: Improved imports with size management capabilities.
The addition of sizeBytesEstimate and size-related constants enhances the frame stream size management, providing fixed boundaries for better resource control.
146-146: Verify thread safety in frame handling.
While the cache initialization is cleaner now, please ensure thread safety when handling frames between the worker and main thread. Consider:
- Race conditions between frame updates and cache operations
- Potential issues during rapid frame seeking
Let's check for synchronization mechanisms:
#!/bin/bash
# Look for worker message handling patterns
ast-grep --pattern 'worker.postMessage({ $$$method: $_, $$$})'
# Check frame handling patterns
ast-grep --pattern 'frameCache.set($_, $_)'53-60: Consider monitoring cache performance metrics.
The cache configuration looks solid with proper size limits and eviction strategy. However, since this is a significant change in caching behavior:
- Consider adding telemetry to monitor:
- Cache hit/miss rates
- Eviction frequency
- Memory usage patterns
- Verify if
MAX_FRAME_STREAM_SIZE_BYTES(1e9) is optimal for your use case
Let's verify the constant usage:
✅ Verification successful
Cache configuration is consistently applied across the codebase
The constants MAX_FRAME_STREAM_SIZE (5100) and MAX_FRAME_STREAM_SIZE_BYTES (1e9) are properly defined in constants.ts and consistently used in both frame-reader and ima-vid-frame-samples caches. The configuration is well-aligned across different cache implementations, with proper size limits and eviction strategies. The original review comment's suggestion for monitoring remains valid but there are no inconsistencies in the constant usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of these constants to ensure consistent limits
rg "MAX_FRAME_STREAM_SIZE" --type ts
rg "MAX_FRAME_STREAM_SIZE_BYTES" --type ts
Length of output: 222
Script:
#!/bin/bash
# Let's search for the constant definitions and their usage
rg "MAX_FRAME_STREAM_SIZE" -A 2 --type ts
rg "MAX_FRAME_STREAM_SIZE_BYTES" -A 2 --type ts
# Also search for any cache-related configuration
ast-grep --pattern 'const $_ = new LRUCache<$_, $_>({$$$})'
Length of output: 4397
app/packages/core/src/components/Grid/Grid.tsx (2)
Line range hint 119-157: Well-implemented effect handling and cleanup
The implementation shows good practices:
- Proper cleanup of video resources
- Debounced resize handling
- Clear separation of layout and regular effects
30-30: Verify impact of removed caching mechanism
The removal of caching from useRefreshers could affect grid performance, especially with the PR's goal of reintroducing size bytes caching. Please ensure this change aligns with the intended caching behavior.
app/packages/looker/src/lookers/imavid/controller.ts (1)
176-176: Verify the fixed cache size implementation.
The removal of maxFrameStreamSize parameter suggests a transition to fixed-size caching. While this aligns with the PR objective of "setting a fixed size of 500 lookers for the grid", we should ensure proper memory management.
Let's verify the new caching implementation:
✅ Verification successful
Let me gather more information about the fixed cache size implementation.
Let me check one final piece to see how the LRUCache is initialized in ImaVidFrameSamples.
Fixed cache size implementation is properly handled
The removal of maxFrameStreamSize parameter is correct as the caching is now managed through global constants:
MAX_FRAME_STREAM_SIZE = 5100for the maximum number of framesMAX_FRAME_STREAM_SIZE_BYTES = 1e9for the maximum cache size in bytes
These constants are properly utilized in the frame reader's LRU cache implementation, ensuring consistent memory management across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the new fixed cache size constant and its usage
# Test 1: Find the new cache size constant
echo "Searching for cache size constant..."
rg -l "MAX_FRAME_STREAM_SIZE" app/packages/looker/src/
# Test 2: Check if the constant is properly used in ImaVidFrameSamples
echo "Checking ImaVidFrameSamples implementation..."
rg -A 10 "class ImaVidFrameSamples" app/packages/looker/src/
Length of output: 1540
Script:
#!/bin/bash
# Check the constants file and its usage
echo "Checking constants.ts content..."
cat app/packages/looker/src/constants.ts
echo -e "\nChecking LRUCache initialization in ImaVidFrameSamples..."
rg -A 5 "samples: LRUCache.*=.*new LRUCache" app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts
echo -e "\nChecking MAX_FRAME_STREAM_SIZE usage in frame-reader..."
rg -B 2 -A 2 "MAX_FRAME_STREAM_SIZE" app/packages/looker/src/lookers/frame-reader.ts
Length of output: 2052
Script:
#!/bin/bash
# Check the complete ImaVidFrameSamples constructor implementation
echo "Checking ImaVidFrameSamples constructor..."
rg -B 5 -A 15 "constructor.*BufferManager" app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts
Length of output: 985
app/packages/state/src/hooks/useCreateLooker.ts (2)
24-25: LGTM: Import statements are properly optimized.
The removal of unused Recoil imports and addition of necessary ones maintains clean dependency management.
Also applies to: 29-29
253-253: Verify frame stream caching behavior after removing maxFrameStreamSize.
The removal of maxFrameStreamSize from the Looker configuration seems to contradict the PR objective of "reintroducing size bytes caching for frame data streams". This could potentially lead to unbounded memory usage.
Let's verify the frame stream caching implementation:
app/packages/looker/src/overlays/classifications.ts (1)
10-10: LGTM: Import addition follows best practices
The sizeBytesEstimate import is appropriately placed and follows the established import grouping pattern.
app/packages/utilities/src/index.ts (1)
14-14: LGTM: Addition of sizeBytesEstimate export.
The addition of sizeBytesEstimate export aligns with the PR objectives to reintroduce size bytes caching for frame data streams. The implementation follows good practices by:
- Using a dedicated module for the implementation
- Following the alphabetical order of exports
- Using proper TypeScript syntax
| @@ -357,86 +355,6 @@ const ShowModalNav = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| const Caching = ({ modal }: { modal?: boolean }) => { | |||
There was a problem hiding this comment.
Added by #5022. After discussion, we decided to remove
|
|
||
| type SizerTypes = SizeTypes | Array<SizeTypes>; | ||
|
|
||
| const sizer = (object: SizerTypes) => { |
There was a problem hiding this comment.
Cleaner implementation of what currently exists
| type SizerTypes = SizeTypes | Array<SizeTypes>; | ||
|
|
||
| const sizer = (object: SizerTypes) => { | ||
| if (object !== null && object !== undefined) { |
There was a problem hiding this comment.
I am confused by this logic. Why are we returning 1 if the object is defined? Also I thought you could just
if (object) {
//do whatever
}
to determine if it's not null or undefined
There was a problem hiding this comment.
Woops! Thank you
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/utilities/src/size-bytes-estimate.test.ts (1)
5-37: Consider adding test cases for important edge cases.While the current test suite covers basic cases, consider adding tests for:
- Empty arrays and objects
- Large strings/arrays to verify performance characteristics
- Nested objects
- Circular references
- Mixed nested structures
This would ensure the size estimation remains accurate in all scenarios.
Example additional test cases:
test("empty array returns 0", () => { expect(sizeBytesEstimate([])).toBe(0); }); test("empty object returns 0", () => { expect(sizeBytesEstimate({})).toBe(0); }); test("nested objects are calculated correctly", () => { expect(sizeBytesEstimate({ a: { b: "test" } })).toBe(10); // Adjust expected value }); test("handles circular references gracefully", () => { const circular = { a: 1 }; circular.self = circular; expect(() => sizeBytesEstimate(circular)).not.toThrow(); });app/packages/utilities/src/size-bytes-estimate.ts (2)
1-14: Consider using Map for better performance and built-in functionalityThe cache implementation could benefit from using
Mapinstead of a plain object for better performance with non-string keys and built-in functionality.-const OBJECT_CLASS_CACHE = {}; +const OBJECT_CLASS_CACHE = new Map<string, string>(); const objectClass = (object: object) => { const prototype = Object.prototype.toString.call(object); - if (!OBJECT_CLASS_CACHE[prototype]) { - OBJECT_CLASS_CACHE[prototype] = prototype.slice(8, -1); + if (!OBJECT_CLASS_CACHE.has(prototype)) { + OBJECT_CLASS_CACHE.set(prototype, prototype.slice(8, -1)); } - return OBJECT_CLASS_CACHE[prototype]; + return OBJECT_CLASS_CACHE.get(prototype); };
20-65: Document size assumptions and limitationsThe function makes specific assumptions about data type sizes (e.g., 8 bytes for numbers and booleans, 2 bytes per character for strings). These assumptions should be documented in JSDoc format for better maintainability.
+/** + * Estimates the size of a value in bytes. + * Assumptions: + * - Numbers and booleans: 8 bytes + * - Strings: 2 bytes per character (UTF-16) + * - Objects: Sum of key and value sizes + * - Arrays: Sum of element sizes + * - ArrayBuffer: Actual byte length + * - null/undefined: 1 byte + * @param object The value to measure + * @returns Estimated size in bytes + */ const sizer = (object: SizerTypes) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/utilities/src/size-bytes-estimate.test.ts(1 hunks)app/packages/utilities/src/size-bytes-estimate.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/utilities/src/size-bytes-estimate.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/utilities/src/size-bytes-estimate.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/utilities/src/size-bytes-estimate.test.ts (2)
1-2: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary testing utilities and the function under test.
4-4: LGTM! Well-structured test suite.
The test suite follows best practices with a descriptive describe block that clearly indicates its purpose.
app/packages/utilities/src/size-bytes-estimate.ts (2)
16-18: LGTM! Well-defined type definitions
The type definitions are clear, comprehensive, and provide good type safety.
67-69: LGTM! Clean and well-typed export
The exported function provides a clean interface to the sizer functionality.
| test("Array values are summed", async () => { | ||
| expect(sizeBytesEstimate([1, "2"])).toBe(10); | ||
| }); | ||
|
|
||
| test("ArrayBuffer returns byteLength", async () => { | ||
| const buffer = new ArrayBuffer(8); | ||
| expect(sizeBytesEstimate(buffer)).toBe(buffer.byteLength); | ||
| }); | ||
|
|
||
| test("boolean returns 8", async () => { | ||
| expect(sizeBytesEstimate(false)).toBe(8); | ||
| }); | ||
|
|
||
| test("null and undefined return 1", async () => { | ||
| expect(sizeBytesEstimate(null)).toBe(1); | ||
| expect(sizeBytesEstimate(undefined)).toBe(1); | ||
| }); | ||
|
|
||
| test("number returns 8", async () => { | ||
| expect(sizeBytesEstimate(1)).toBe(8); | ||
| }); | ||
|
|
||
| test("objects entries are summed", async () => { | ||
| expect(sizeBytesEstimate({ "1": 1 })).toBe(10); | ||
| }); | ||
|
|
||
| test("string returns chars*2", async () => { | ||
| expect(sizeBytesEstimate("chars")).toBe(10); | ||
| }); | ||
|
|
||
| test("non-objects return 0", async () => { | ||
| expect(sizeBytesEstimate(() => null)).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async markers from synchronous tests.
The test cases are marked as async but don't contain any asynchronous operations. Remove the async markers to improve clarity.
- test("Array values are summed", async () => {
+ test("Array values are summed", () => {
- test("ArrayBuffer returns byteLength", async () => {
+ test("ArrayBuffer returns byteLength", () => {
- test("boolean returns 8", async () => {
+ test("boolean returns 8", () => {
- test("null and undefined return 1", async () => {
+ test("null and undefined return 1", () => {
- test("number returns 8", async () => {
+ test("number returns 8", () => {
- test("objects entries are summed", async () => {
+ test("objects entries are summed", () => {
- test("string returns chars*2", async () => {
+ test("string returns chars*2", () => {
- test("non-objects return 0", async () => {
+ test("non-objects return 0", () => {📝 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.
| test("Array values are summed", async () => { | |
| expect(sizeBytesEstimate([1, "2"])).toBe(10); | |
| }); | |
| test("ArrayBuffer returns byteLength", async () => { | |
| const buffer = new ArrayBuffer(8); | |
| expect(sizeBytesEstimate(buffer)).toBe(buffer.byteLength); | |
| }); | |
| test("boolean returns 8", async () => { | |
| expect(sizeBytesEstimate(false)).toBe(8); | |
| }); | |
| test("null and undefined return 1", async () => { | |
| expect(sizeBytesEstimate(null)).toBe(1); | |
| expect(sizeBytesEstimate(undefined)).toBe(1); | |
| }); | |
| test("number returns 8", async () => { | |
| expect(sizeBytesEstimate(1)).toBe(8); | |
| }); | |
| test("objects entries are summed", async () => { | |
| expect(sizeBytesEstimate({ "1": 1 })).toBe(10); | |
| }); | |
| test("string returns chars*2", async () => { | |
| expect(sizeBytesEstimate("chars")).toBe(10); | |
| }); | |
| test("non-objects return 0", async () => { | |
| expect(sizeBytesEstimate(() => null)).toBe(0); | |
| }); | |
| test("Array values are summed", () => { | |
| expect(sizeBytesEstimate([1, "2"])).toBe(10); | |
| }); | |
| test("ArrayBuffer returns byteLength", () => { | |
| const buffer = new ArrayBuffer(8); | |
| expect(sizeBytesEstimate(buffer)).toBe(buffer.byteLength); | |
| }); | |
| test("boolean returns 8", () => { | |
| expect(sizeBytesEstimate(false)).toBe(8); | |
| }); | |
| test("null and undefined return 1", () => { | |
| expect(sizeBytesEstimate(null)).toBe(1); | |
| expect(sizeBytesEstimate(undefined)).toBe(1); | |
| }); | |
| test("number returns 8", () => { | |
| expect(sizeBytesEstimate(1)).toBe(8); | |
| }); | |
| test("objects entries are summed", () => { | |
| expect(sizeBytesEstimate({ "1": 1 })).toBe(10); | |
| }); | |
| test("string returns chars*2", () => { | |
| expect(sizeBytesEstimate("chars")).toBe(10); | |
| }); | |
| test("non-objects return 0", () => { | |
| expect(sizeBytesEstimate(() => null)).toBe(0); | |
| }); |
| if (!Array.isArray(object)) { | ||
| let size = 0; | ||
| for (const key in object) { | ||
| console.log("KEY", key); |
There was a problem hiding this comment.
Remove debug console.log statement
Production code should not contain debug console.log statements.
- console.log("KEY", key);📝 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.
| console.log("KEY", key); |
a8a4c26 to
09d21e0
Compare
What changes are proposed in this pull request?
Adds (back) size bytes caching for frame data streams (ImaVid and VideoLooker) and fixed size of 500 lookers for the grid. This more closely matches caching behavior prior to routing and grid updates released in 0.25
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
sizeBytesEstimatefunction for estimating the size of various data types.getSizeBytesmethods toOverlayandClassificationsOverlayclasses for size estimation.Changes
VideoConfiginterface to remove themaxFrameStreamSizeproperty.Bug Fixes