Grid caching feedback #5548
Grid caching feedback #5548
Conversation
- Rename useAt to useScrollLocation - Document zoom -> aspect ratio relationship -
- Remove Entry type for hidden cause and use a shown check on dipose - Use constants where possible - Document aspect ratio recommendation 4x heuristic
WalkthroughThis pull request updates multiple components across the codebase by replacing outdated constants with new, more descriptive ones and renaming several hooks and interfaces for clarity. Key changes include updating link references, revising grid functionalities such as scroll location and zoom settings, refactoring cache logic (including lifecycle hooks), and streamlining error handling through modular Snackbar components. Changes in the Spotlight module adjust measurement and rendering flows, while new documentation and state atoms have been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Grid
participant useScrollLocation
participant useZoomSetting
User->>Grid: Interacts with grid (scroll/zoom)
Grid->>useScrollLocation: Request scroll position
Grid->>useZoomSetting: Request zoom setting
useScrollLocation-->>Grid: Returns scroll location
useZoomSetting-->>Grid: Returns zoom value
Grid->>Grid: Update layout and cache calculations
sequenceDiagram
participant Document
participant Cache
Document->>Cache: visibilitychange event triggered
Cache->>Cache: Execute cache.empty() to clear cache
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
benjaminpkane
force-pushed
the
grid-caching-feedback
branch
from
2da2ec0 to
d8f95f8
Compare
benjaminpkane
force-pushed
the
grid-caching-feedback
branch
from
31012e1 to
ab3bd5e
Compare
benjaminpkane
force-pushed
the
grid-caching-feedback
branch
from
90bc49b to
995cf9e
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: 0
🧹 Nitpick comments (1)
app/packages/spotlight/src/index.ts (1)
127-134: Use ofResizeObserverandrequestAnimationFrame
Re-rendering upon resize followed by an animation-frame-based measurement is logical, but consider debouncing if frequent resizes create performance overhead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
docs/source/images/app/app-managing-grid-memory.gifis excluded by!**/*.gif,!**/*.gife2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-hide-person-cat-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-hide-person-cat-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-show-cat-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar.spec.ts-snapshots/select-bottle-show-cat-chromium-linux.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (27)
app/packages/core/src/components/Actions/Options.tsx(3 hunks)app/packages/core/src/components/Grid/Grid.tsx(5 hunks)app/packages/core/src/components/Grid/index.ts(1 hunks)app/packages/core/src/components/Grid/recoil.ts(6 hunks)app/packages/core/src/components/Grid/useEvents.ts(4 hunks)app/packages/core/src/components/Grid/useFontSize.ts(1 hunks)app/packages/core/src/components/Grid/useLabelVisibility.ts(1 hunks)app/packages/core/src/components/Grid/useLookerCache.ts(1 hunks)app/packages/core/src/components/Grid/useRenderer.ts(1 hunks)app/packages/core/src/components/Grid/useScrollLocation.ts(2 hunks)app/packages/core/src/components/Grid/useZoomSetting.ts(2 hunks)app/packages/core/src/components/ImageContainerHeader.tsx(2 hunks)app/packages/core/src/components/QueryPerformanceToast.tsx(2 hunks)app/packages/core/src/components/Sidebar/Entries/QueryPerformanceIcon.tsx(2 hunks)app/packages/core/src/components/Snackbar.tsx(1 hunks)app/packages/core/src/utils/links.ts(1 hunks)app/packages/looker/src/cache.test.ts(6 hunks)app/packages/looker/src/cache.ts(8 hunks)app/packages/spotlight/src/constants.ts(1 hunks)app/packages/spotlight/src/index.ts(13 hunks)app/packages/spotlight/src/row.ts(2 hunks)app/packages/spotlight/src/section.ts(1 hunks)app/packages/spotlight/src/types.ts(2 hunks)app/packages/spotlight/src/utilities.ts(0 hunks)app/packages/state/src/recoil/atoms.ts(1 hunks)docs/source/user_guide/app.rst(1 hunks)fiftyone/core/config.py(1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/spotlight/src/utilities.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{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/useLookerCache.tsapp/packages/spotlight/src/constants.tsapp/packages/core/src/components/ImageContainerHeader.tsxapp/packages/core/src/components/Grid/useFontSize.tsapp/packages/core/src/components/Grid/useRenderer.tsapp/packages/core/src/components/QueryPerformanceToast.tsxapp/packages/core/src/utils/links.tsapp/packages/core/src/components/Sidebar/Entries/QueryPerformanceIcon.tsxapp/packages/state/src/recoil/atoms.tsapp/packages/core/src/components/Grid/useLabelVisibility.tsapp/packages/spotlight/src/section.tsapp/packages/spotlight/src/row.tsapp/packages/core/src/components/Grid/useZoomSetting.tsapp/packages/spotlight/src/types.tsapp/packages/looker/src/cache.test.tsapp/packages/core/src/components/Snackbar.tsxapp/packages/core/src/components/Actions/Options.tsxapp/packages/core/src/components/Grid/useScrollLocation.tsapp/packages/core/src/components/Grid/recoil.tsapp/packages/core/src/components/Grid/index.tsapp/packages/spotlight/src/index.tsapp/packages/core/src/components/Grid/useEvents.tsapp/packages/looker/src/cache.tsapp/packages/core/src/components/Grid/Grid.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (94)
fiftyone/core/config.py (1)
355-355: Increased default grid zoom value.The default value for
grid_zoomhas been changed from 5 to 7, which will result in a higher default zoom level in the grid view. This aligns with the application's zoom settings refactoring.app/packages/core/src/components/Grid/useLabelVisibility.ts (1)
6-8: Improved code documentation.The added documentation comment clearly explains the purpose of this module, which helps improve code maintainability by explicitly stating that these callbacks are for synchronizing the
gridActivePathsLUTcache.app/packages/spotlight/src/constants.ts (2)
16-16: Added DEFAULT_MAX_ROWS constant.The new constant
DEFAULT_MAX_ROWSwith a value of 80 follows good programming practice by replacing a magic number with a named constant, improving code readability and maintainability.
23-23: Added MIN_ASPECT_RATIO_RECOMMENDATION constant.The new constant
MIN_ASPECT_RATIO_RECOMMENDATIONwith a value of 1.5 eliminates a hardcoded value, making the code more maintainable and the purpose of this threshold more clear.app/packages/core/src/components/Grid/index.ts (1)
2-3: Updated exported zoom-related entities.The exports have been updated to replace
gridZoomRangewithgridZoomfrom "./recoil" and addedZOOM_RANGEfrom "./useZoomSetting". This change is part of the refactoring of the zoom management logic across the application.app/packages/core/src/components/Grid/useLookerCache.ts (1)
18-22: Good addition for cache management during visibility changes.This implementation effectively clears the cache when the document visibility changes (e.g., when switching tabs or minimizing the window), which helps manage memory usage. The effect is properly set up with a cleanup function that removes the event listener on unmount.
app/packages/core/src/components/Grid/useRenderer.ts (1)
52-52:✅ Verification successful
Consistent Promise return type for better type safety.
The change from returning
0directly to returningPromise.resolve(0)ensures a consistent return type for theshowItemfunction across all code paths. This improvement helps with TypeScript type safety and creates a more predictable API for consumers of this function.
🏁 Script executed:
#!/bin/bash # Verify if there are any direct consumers of this function that might expect a number return type rg -A 5 -B 5 "showItem\(" --type=tsLength of output: 690
Attention: Consistent Promise Return Verified
The change to return
Promise.resolve(0)indeed enforces a consistent asynchronous return type (i.e.,Promise<number>) across all code paths. Our verification showed that consumers (as seen inapp/packages/spotlight/src/row.ts) do not rely on a synchronous number but instead properly interact with the promise-based API. No additional modifications are necessary.app/packages/spotlight/src/section.ts (1)
136-136:✅ Verification successful
Made measure parameter optional for increased flexibility.
Making the
measureparameter optional improves the flexibility of therendermethod. This change aligns with modern TypeScript practices of making parameters optional when they don't need to be required in all scenarios.
🏁 Script executed:
#!/bin/bash # Check if Row.show also handles optional measure parameter consistently rg -A 5 "show\(\{" app/packages/spotlight/src/row.tsLength of output: 127
Code change approved: Optional measure parameter is consistently handled.
The verification confirms that theRow.showmethod inapp/packages/spotlight/src/row.tsdestructures themeasureproperty from its parameter object. Although the destructuring syntax inRow.showdoes not explicitly show the optional marker (?), TypeScript ensures that an optional property is correctly typed if its definition in the interface or function parameter type marks it as such. The updated optionalmeasureinsection.tsaligns with the intended increased flexibility and modern TypeScript practices without disrupting consistency between modules.app/packages/state/src/recoil/atoms.ts (1)
114-117:✅ Verification successful
Well-structured addition of snackbarLink atom.
This new atom provides a clean way to manage link notifications in the application state. The type is appropriately defined to include both the link URL and the message text. This addition suggests an enhancement to the notification system to support clickable links in toast messages, which improves user experience.
🏁 Script executed:
#!/bin/bash # Check for implementations that use this new snackbarLink atom rg "snackbarLink" --type=tsLength of output: 512
Integration Confirmed: Clean Implementation of snackbarLink Atom
The addition of the
snackbarLinkatom is well-implemented and integrates seamlessly with the notification system. Verification shows that:
- Defined in:
app/packages/state/src/recoil/atoms.ts- Referenced in:
app/packages/core/src/components/Snackbar.tsx(usinguseRecoilState)app/packages/core/src/components/Grid/useEvents.ts(usinguseSetRecoilState)app/packages/core/src/components/Grid/recoil.ts(usingset)The type definition and default value are correctly set, ensuring clickable link notifications enhance the user experience.
app/packages/core/src/components/Grid/useFontSize.ts (3)
2-2: Good addition of useZoomSetting importThis change appropriately replaces the old threshold system with the new zoom setting implementation, making the code more consistent with other parts of the application.
9-9: Appropriate replacement of threshold with zoomGood refactoring to use the more descriptively named
zoomfunction from theuseZoomSettinghook instead of the previousthreshold.
17-18: Clean implementation of zoom setting in font size calculationThe calculation has been updated to use
zoom(width)instead ofthreshold(width)and dependencies have been appropriately updated. This creates better consistency with the grid caching system.app/packages/core/src/components/QueryPerformanceToast.tsx (2)
3-3: Improved constant naming with more descriptive importsGood replacement of
QP_MODEandQP_MODE_SUMMARYwith the more descriptiveOPTIMIZING_QUERY_PERFORMANCEandSUMMARY_FIELDS, making the code more self-documenting.
46-47: Updated link references with better named constantsThe implementation correctly uses the new constant names while maintaining the same functionality, providing better context about what these links represent.
app/packages/core/src/components/Sidebar/Entries/QueryPerformanceIcon.tsx (2)
2-2: Improved constant naming with more descriptive importExcellent replacement of
QP_MODEwith the more descriptiveOPTIMIZING_QUERY_PERFORMANCE, making the code more self-documenting and consistent with other parts of the codebase.
71-73: Updated documentation link reference with better named constantThe implementation correctly uses the new constant name while maintaining the same functionality. This makes the purpose of the link clearer for developers.
app/packages/core/src/components/ImageContainerHeader.tsx (4)
7-7: Simplified Recoil imports and added constSelectorGood optimization by removing
useRecoilCallbackand addingconstSelector, which streamlines the component's implementation.
11-11: Updated imports for improved zoom functionalityAppropriate replacement of
gridZoomRangewithZOOM_RANGE, reflecting the shift to using a constant range for zoom settings.
87-87: Simplified resetZoom implementationExcellent refactoring that simplifies the
resetZoomfunction by directly usinguseResetRecoilState(gridZoom)instead of a more complex asynchronous implementation. This makes the code cleaner and more maintainable.
95-95: Updated slider bounds with constant selectorGood implementation change to use
constSelector(ZOOM_RANGE)for the slider bounds, which is consistent with the new approach of using a constant zoom range.app/packages/core/src/components/Grid/useZoomSetting.ts (2)
9-18: Clear documentation of the zoom logic - well done!The added JSDoc comments provide excellent clarity on how the aspect ratio and zoom range work together. The explanation that smaller aspect ratios result in more zoom and larger ones in less zoom makes the slider behavior intuitive for users.
29-39: Improved zoom range with better defaultsThe updated zoom range values provide a more appropriate scaling for different viewport widths. Using negative values and inverting the zoom logic (where more negative means more zoom) is a good approach for this particular UI control.
app/packages/looker/src/cache.test.ts (5)
12-18: Good addition of lifecycle hooks to cache creationAdding the
onDisposeandonSetcallback functions as parameters to the cache creation improves the API design by allowing subscribers to react to important cache lifecycle events.
27-28: Added appropriate test for onSet callbackThe test now properly verifies that the
onSetcallback is triggered when an item is added to the cache.
43-44: Improved type safety by removing non-null assertionRemoving the non-null assertion (
!) improves type safety by forcing proper handling of potential undefined values when retrieving items from the cache.
71-72: Thorough test coverage for subsequent set operationsAdding verification for multiple calls to the
onSetcallback ensures the cache behaves correctly when setting items that already exist.
89-90: Verifying disposal callback executionTesting that the
onDisposecallback is triggered when the cache is deleted ensures proper resource cleanup, which is crucial for memory management.docs/source/user_guide/app.rst (1)
398-422: Excellent documentation for the grid memory management featureThe new section provides clear and comprehensive information about how the grid cache works, including:
- Default cache size based on device memory
- Explanation of autosizing functionality
- Use cases where this feature is particularly beneficial
- Instructions for disabling autosizing if needed
This will help users understand the performance characteristics of the application when working with large datasets.
app/packages/spotlight/src/row.ts (2)
161-161: Good API improvement by making measure parameter optionalMaking the
measureparameter optional improves the API's flexibility and allows for cases where measurement isn't needed, reducing unnecessary overhead.
177-185: Improved measurement flow with direct value captureThe refactored code now directly captures the bytes value returned from
showItemand conditionally calls the measure function. This is cleaner than the previous callback approach and provides better control over the execution flow.This change aligns well with the grid caching improvements mentioned in the PR objectives.
app/packages/core/src/utils/links.ts (3)
16-17: LGTM: Updated documentation link for grid memory managementThe new constant
MANAGING_GRID_MEMORYprovides a more specific and descriptive name compared to the previous constant it replaces, clearly indicating its purpose for grid memory management documentation.
24-25: LGTM: Updated documentation link for query performance optimizationThe new constant
OPTIMIZING_QUERY_PERFORMANCEreplaces the previous constants related to query performance with a more descriptive name, improving clarity about its purpose.
33-34: LGTM: Added new documentation link for summary fieldsThe new constant
SUMMARY_FIELDSprovides a direct link to documentation about summary fields, enhancing the application's documentation resources.app/packages/core/src/components/Actions/Options.tsx (3)
17-20: LGTM: Updated imports to use new documentation link constantsThe import statement correctly includes the new constants from the links utility file, replacing the previous constants that were removed.
205-205: LGTM: Updated href to use new query performance documentation linkThe
hrefproperty in the QueryPerformance component now correctly uses the newOPTIMIZING_QUERY_PERFORMANCEconstant.
298-299: LGTM: Updated href to use new grid memory management documentation linkThe
hrefproperty in the Grid component now correctly uses the newMANAGING_GRID_MEMORYconstant.app/packages/core/src/components/Grid/Grid.tsx (6)
25-25: LGTM: Updated imports with more descriptive hook namesThe imports now use more semantically meaningful names (
useScrollLocationanduseZoomSetting) that better reflect the purpose of these hooks.Also applies to: 28-28
39-39: LGTM: Renamed threshold to zoom for better clarityThe variable name change from
thresholdtozoommakes the code more intuitive by using terminology that better describes what the variable represents.
45-46: LGTM: Added clarifying comment about memory allocationThe updated comment explicitly describes how the memory is divided between hidden cache and visible items, providing better context for the division operation.
67-67: LGTM: Updated to use renamed scroll location hookNow using the more descriptively named
useScrollLocationhook instead of the previoususeAt, improving code readability.
94-94: LGTM: Updated property name to match new zoom variableThe
rowAspectRatioThresholdproperty now correctly uses the renamedzoomvariable.
107-107: LGTM: Updated dependency array with new variable nameThe dependency array in the
useMemoOnehook has been properly updated to include the renamedzoomvariable, ensuring the memoization works correctly.app/packages/core/src/components/Grid/useScrollLocation.ts (3)
6-6: LGTM: Renamed interface for better semantic clarityThe interface has been renamed from
AtInterfacetoScrollLocation, which better describes its purpose and improves code readability.
12-12: LGTM: Renamed hook function for better semantic clarityThe hook function has been renamed from
useAttouseScrollLocation, which better describes its purpose and follows React hook naming conventions.
61-65: LGTM: Updated type annotation and property access to use renamed interfaceThe parameter type and property access in the
setfunction have been properly updated to use the renamedScrollLocationinterface, maintaining consistency throughout the codebase.app/packages/core/src/components/Grid/useEvents.ts (6)
9-9: Good addition of a descriptive constant for the link.The introduction of the
MANAGING_GRID_MEMORYconstant improves code maintainability by centralizing link references.
13-13: Improved type naming from AtInterface to ScrollLocation.This rename provides better semantic clarity about what the type represents.
27-27: Updated parameter type in function signature.The parameter type update from
AtInterfacetoScrollLocationmaintains consistency with the imported type change.
30-31: Enhanced error handling with improved descriptive names.The changes from
handleErrortohandleAutosizeandsetRecommendedGridZoomRangetosetRecommendedZoomimprove code readability and align better with their actual functions.
57-62: Improved user experience with better error messages and links.The refactored error handling now provides more context to users by including a helpful link to documentation about memory management, which is a significant UX improvement over the previous implementation.
82-87: Updated dependency array for useLayoutEffect.The dependency array has been correctly updated to match the renamed variables, which is essential for proper hook behavior.
app/packages/core/src/components/Grid/recoil.ts (8)
6-14: Well-documented utility function for zoom range conversion.The addition of the
convertDefaultfunction with clear JSDoc comments is excellent. It cleanly transforms zoom settings between different ranges, making the codebase more maintainable.
23-23: Simplified default value for gridAutosizingStore.Changing from a conditional expression to a simple
truevalue simplifies the code while maintaining functionality.
62-70: Good implementation of per-dataset zoom settings.The introduction of
gridZoomStoreas an atomFamily allows for better state management by storing zoom settings per dataset rather than globally, which improves the application's ability to remember user preferences per dataset.
76-78: Improved zoom setting logic.The updated zoom retrieval logic that uses the new
gridZoomStorewith a fallback to the converted default value makes the code more robust.
90-93: Enhanced zoom value handling with DefaultValue support.The code now properly handles DefaultValue instances from Recoil, falling back to the converted default zoom when needed, which improves state reset behavior.
97-100: Better user feedback with structured error messages.Replacing error arrays with a structured object containing a link and message provides clearer feedback to users and directs them to helpful resources.
105-105: Updated state management to use gridZoomStore.Setting values in the dataset-specific
gridZoomStoreinstead of the globalstoredGridZoomimproves state isolation between datasets.
135-135:✅ Verification successful
Changed memory allocation calculation.
The divisor was changed from 16 to 8, which effectively doubles the default maximum memory allocation for grid items. Ensure this change is intentional and won't cause performance issues on devices with limited memory.
🌐 Web query:
What are typical device memory values for modern browsers and how does allocating (deviceMemory / 8) * 1e9 bytes compare to available system memory?💡 Result:
Modern browsers use the Device Memory API to provide approximate values of a device's RAM capacity through
navigator.deviceMemory, with standardized values of 0.25, 0.5, 1, 2, 4, or 8 GB[4][10][19]. These values are rounded down to the nearest power of two for privacy reasons[4][10].Key Comparisons
For the formula
(deviceMemory / 8) * 1e9 bytes:
- 8 GB device: Allocates 1 GB (12.5% of reported RAM).
- 4 GB device: Allocates 0.5 GB (12.5% of reported RAM).
- 2 GB device: Allocates 0.25 GB (12.5% of reported RAM).
This allocation represents a conservative fraction of the reported device memory, not the actual system memory. Since
deviceMemoryitself is a lower-bound approximation (e.g., 3 GB RAM devices report 2 GB[10]), the formula further scales down usage to avoid overconsumption.Practical Implications
System Memory vs. Allocation:
Actual system memory is typically higher thandeviceMemoryvalues (e.g., a device with 6 GB RAM reports 4 GB via the API[10]). The formula’s output is thus a small fraction of the true available RAM.Browser Memory Management:
Browsers like Chrome partition memory per process[11][16], and the allocation from this formula would represent a single process’s budget. For context, Chrome’s per-tab memory usage averages ~100–200 MB[16], making this formula’s allocation suitable for lightweight tasks but potentially restrictive for memory-heavy applications.Use Cases:
Developers might use such a formula to:
- Limit resource-intensive features (e.g., high-resolution images) on low-memory devices[12][15].
- Avoid crashes on devices with ≤1 GB RAM[16].
Limitations
- Privacy Constraints: The API’s 8 GB upper bound[10] means high-end devices (e.g., 32 GB RAM) are treated as 8 GB, capping allocations at 1 GB regardless of actual capacity.
- No Direct Correlation to Free Memory: The formula doesn’t account for memory already in use by the OS or other applications[6][16].
For context, here’s how allocations compare across common
deviceMemoryvalues:
deviceMemory(GB)Allocated Bytes Equivalent 8 1,000,000,000 1 GB 4 500,000,000 500 MB 2 250,000,000 250 MB 0.5 62,500,000 62.5 MB This approach balances compatibility with privacy safeguards but requires careful optimization for memory-sensitive applications.
Citations:
- 1: https://web.dev/articles/monitor-total-page-memory-usage
- 2: https://www.thyngster.com/step-by-step-guide-measuring-javascript-memory-usage-on-your-web-pages
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Device_Memory_API
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Performance/measureUserAgentSpecificMemory
- 6: https://www.techtarget.com/whatis/definition/memory
- 7: https://modheader.com/usecases/headers/device-memory
- 8: https://docs.singlestore.com/db/v8.9/user-and-cluster-administration/high-availability-and-disaster-recovery/replicating-data-across-clusters/calculating-memory-allocation-per-host-in-a-cluster/
- 9: https://forums.developer.nvidia.com/t/16-1-run-time-out-of-memory-allocating-x-byte-device-memory/134848
- 10: https://www.w3.org/TR/device-memory/
- 11: https://www.chromium.org/developers/memory-usage-backgrounder/
- 12: https://developer.arm.com/documentation/ddi0406/b/Application-Level-Architecture/Application-Level-Memory-Model/Memory-types-and-attributes-and-the-memory-order-model/Device-memory
- 13: https://developer.chrome.com/blog/device-memory
- 14: https://dev.to/iprosk/playing-with-low-level-memory-in-webassembly-257i
- 15: https://dev.to/martygo/mastering-web-apis-device-memory-api-25ji
- 16: https://www.ninjaone.com/blog/chrome-high-ram-usage/
- 17: https://developer.chrome.com/docs/devtools/memory-problems
- 18: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Memory_management
- 19: https://www.holisticseo.digital/technical-seo/web-accessibility/http-header/device-memory
- 20: https://downloads.wavefun.com/FAQ/Memory_FAQ.html
Memory Allocation Revision Verification
The updated calculation now uses
(deviceMemory / 8) * 1e9to set the default maximum memory allocation. Based on typicalnavigator.deviceMemoryvalues (0.25, 0.5, 1, 2, 4, or 8 GB), this change allocates roughly 12.5% of the reported memory—for example, about 1 GB for an 8 GB device and 500 MB for a 4 GB device. This conservative fraction is in line with the Device Memory API’s approximations and should remain within safe limits for most devices. However, please verify that this increased allocation still performs optimally on lower-memory devices (e.g., on a device reporting 2 GB, this sets the allocation to 250 MB).app/packages/core/src/components/Snackbar.tsx (6)
10-18: Good component extraction for external links.The new
Linkcomponent enhances UX by clearly indicating external links with the Launch icon, improving user understanding of where links will lead.
20-24: Centralized layout configuration improves maintainability.Extracting the layout configuration into a constant
LAYOUThelps maintain consistent positioning and avoids duplication across components.
26-48: Well-structured Dismiss component extraction.Extracting the dismiss button into its own component improves reusability and maintainability. The styling is consistent with the application's theme.
50-64: Good separation of concerns for error handling.The
SnackbarErrorscomponent cleanly separates error message display logic, making the code more modular and easier to maintain.
66-84: Well-implemented links component.The
SnackbarLinkscomponent follows the same pattern asSnackbarErrors, providing consistency in the codebase while adding new functionality for link display.
86-93: Clean composition of Snackbar components.The main
Snackbarcomponent now uses a React Fragment to render bothSnackbarErrorsandSnackbarLinks, which is a clean approach to composition.app/packages/spotlight/src/types.ts (4)
43-43: Simplified API by using Promise directly.Changing
sizeBytesfrom a function returning a promise to directly being a promise simplifies the API and makes the code more straightforward.
65-65: Consistent Promise-based return type.Updating the return type to always be
Promise<number>instead ofnumber | Promise<number>ensures consistency and makes the code more predictable.
73-75: Improved property organization.Reordering the optional properties improves code readability and organization without affecting functionality.
82-82: Consistent property placement.Moving the
showItemproperty to a better position in the interface improves code organization and readability.app/packages/looker/src/cache.ts (11)
40-41: Use ofshownmap looks correct
No immediate issues or concerns with this approach.
43-44: Thependingmap is clearly named
This makes it easy to distinguish between items awaiting size estimates and other caches.
47-49: Dispose callback checks for 'shown' presence
This ensures that items are not destroyed if they remain in theshownmap, avoiding accidental cleanup.
56-56: Size calculation usage ofgetSizeBytesEstimate
The method call aligns with theInstanceinterface and appears correct.
63-63: Order of retrieval might affect instance usage
You switched to prioritizingfrozen, thenhidden,pending, and finallyshown. Please confirm this new order is intentional and aligns with the desired caching logic.
79-79: Conditional set inhidden
The instance is assigned tohiddenonly if not already present. This is a sensible safeguard for avoiding double-updating.
115-118: Different handling of 'hidden' vs other caches
Here,hiddenis cleared directly whilefrozen,pending, andshownare destroyed via thedestroy()helper. If.clear()on the LRU cache does not calldispose(), some items might bypass destruction. Please verify whether disposal is triggered or if this could cause memory leaks.
124-125:empty()method also callshidden.clear()
As above, confirm that this does not skip thedisposeroutine for thehiddenitems. Otherwise, consider manually destroying them.
172-173: Removing existing entries from 'hidden' and 'pending'
Deleting keys from these caches prevents stale references. Looks consistent.
209-215:destroyhelper systematically disposes items
It properly calls each instance'sdestroy()method and then invokesonDispose. This is well-structured for cache cleanup.
224-224: UsingPromise.resolve()for loaded lookers
Short-circuiting loaded items is a clean way to bypass asynchronous overhead.app/packages/spotlight/src/index.ts (14)
10-10: Import ofDEFAULT_MAX_ROWS
No issues. This constant is neatly introduced and used.
16-16: Import ofMIN_ASPECT_RATIO_RECOMMENDATION
This import clarifies the source of the recommended ratio, improving maintainability.
19-19: Import ofTHREE
Likely used for calculations or partial offsets. Straightforward addition.
68-68: UsingDEFAULT_MAX_ROWSin config
Merging the config with this default value is consistent with user-overridable defaults.
196-241: Adaptive aspect-ratio logic for memory usage
Inside#handleHighMemoryUsage, you iteratively reduce the proposed ratio if the estimated memory usage is high. The do-while pattern is valid, but you might consider a more direct or binary-search approach if you want fewer iteration steps. Otherwise, no issues.
264-264: Injection of#measure()results in the scroll reader callback
Passing in measurement info to#renderensures dynamic layout can adjust to scrolling.
269-337: New#measure()function
It provides a promise-based measurement workflow that monitors total bytes to avoid overuse. Consider whether any additional error logic is needed for rejected promises.
341-341: Addedcloseparam to#render
This optional callback can help finalize asynchronous tasks post-render, no issues identified.
350-350: Optionalclosecallback
Exposing a closure hook fosters extensibility. Implementation looks straightforward.
415-415: Invokingclose()after rendering
Invokingclose?.()ensures any cleanup or finalization logic runs following a successful render.
446-455: Refactor: measure first, then render, plus re-render on 'load'
This sequence ensures accurate layout once items are measured. Keep an eye on the additional renders and potential performance overhead.
505-505: Extending#measure()results into the render flow
Consistent usage of the measurement object. No concerns.
579-579:#measure()usage in theprevious()flow
Aligns with the updated measurement approach to manage memory usage and layout.
623-623:#measure()usage for offset rendering
Ensures consistent measurement-based rendering in the#previouslogic.
What changes are proposed in this pull request?
Follow up for #5214
Includes a
Managing Grid Memory Usagedoc section, concurrent loading of samples when initially populating the grid, cache fixes, and a simplified zoom setting (converting between an aspect ratio and a user defined 1-10 value)How is this patch tested? If it is not, please explain why.
Cache tests
What areas of FiftyOne does this PR affect?
fiftyonePython library changes