Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Require memoizee and memoizeClear calls have a max size #2375

Merged
merged 1 commit into from
Mar 3, 2025

Conversation

mattrunyon
Copy link
Collaborator

Related to #2374 (and might need that change before this can merge). This requires all memoizee and memoizeClear calls to provide a max cache size. Most of these were small caches most likely, but this should help prevent another issue that is hard to spot since it takes potentially hours to reproduce like DH-18798

@mattrunyon mattrunyon requested review from a team and Copilot February 27, 2025 00:36
@mattrunyon mattrunyon self-assigned this Feb 27, 2025
@mattrunyon mattrunyon requested review from vbabich and removed request for a team February 27, 2025 00:36

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR refactors memoization calls by requiring that all memoizee and memoizeClear calls explicitly provide a maximum cache size to help prevent long-term caching issues. The changes include converting function expressions to proper function declarations in some cases and adding { max: } options across multiple components and modules.

  • Refactored memoizeClear in packages/grid
  • Added max cache size configurations to memoization calls in grid, iris-grid, dashboard, code-studio, and other packages

Reviewed Changes

File Description
packages/grid/src/memoizeClear.ts Converted the memoizeClear export to a named function and applied memoizee with max config
packages/grid/src/MockTreeGridModel.ts Added { max: 10000 } to each memoizeClear call for caching functions
packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.tsx Updated memoization calls to include specific max values
packages/code-studio/src/settings/ColumnSpecificSectionContent.tsx Updated memoization for ColumnTypeOptions with an added max size configuration
packages/iris-grid/src/sidebar/RollupRows.tsx Provided max cache size options for memoize calls ensuring consistency
packages/chart/src/FigureChartModel.ts Added max parameters to various memoize calls for chart formatting and value translation
packages/iris-grid/src/IrisGrid.tsx Added { max: } options to multiple memoize calls handling grid operations
packages/code-studio/src/styleguide/DraggableLists.tsx Updated memoization calls in event handlers and component producers with max size options
packages/components/src/AutoCompleteInput.tsx Applied a max cache size to the memoized filter function
packages/dashboard-core-plugins/src/panels/DropdownFilterPanel.tsx Updated memoized functions with a max option for caching values
packages/dashboard-core-plugins/src/controls/input-filter/InputFilter.tsx Added max configuration to memoizee call for generating item labels
packages/dashboard-core-plugins/src/controls/dropdown-filter/DropdownFilter.tsx Updated memoization to include an explicit max configuration
packages/code-studio/src/settings/FormattingSectionContent.tsx Added max configuration to memoized formatting components
packages/components/src/ItemList.tsx Updated multiple memoization calls for outer and inner component creation with max options
packages/iris-grid/src/IrisGridPartitionSelector.tsx Added max cache size options to memoizee calls for partition selector callbacks

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 53.06122% with 23 lines in your changes missing coverage. Please review.

Project coverage is 46.80%. Comparing base (3f74d44) to head (d9845de).

Files with missing lines Patch % Lines
...-plugins/src/controls/input-filter/InputFilter.tsx 0.00% 9 Missing ⚠️
packages/iris-grid/src/IrisGrid.tsx 41.66% 7 Missing ⚠️
...rd-core-plugins/src/panels/DropdownFilterPanel.tsx 55.55% 4 Missing ⚠️
...ckages/iris-grid/src/IrisGridPartitionSelector.tsx 0.00% 2 Missing ⚠️
packages/iris-grid/src/sidebar/RollupRows.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2375   +/-   ##
=======================================
  Coverage   46.80%   46.80%           
=======================================
  Files         710      710           
  Lines       39226    39231    +5     
  Branches     9975     9975           
=======================================
+ Hits        18358    18361    +3     
- Misses      20814    20816    +2     
  Partials       54       54           
Flag Coverage Δ
unit 46.80% <53.06%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mattrunyon mattrunyon force-pushed the require-memoize-max-size branch from f96ad6c to d9845de Compare February 27, 2025 16:21
@mattrunyon mattrunyon merged commit bc94430 into deephaven:main Mar 3, 2025
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants