Kimirock/metabase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reduce the frontend to one memoize helper, and stop top-level caches leaking (#81866)

Browse Source 
* Add a jest project for heap retention specs

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Scope the parsed x value cache to one getXValues pass

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Assert heap retention on the settled pass, not the first touch

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Key the log timestamp cache on the log object

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Key the schema viewer flow graph on the response object

The memo key serialized every table and field of the schema, so each distinct
schema held a megabyte-scale string forever. RTK Query returns a stable
reference per cache entry, so a WeakMap keeps the identity that useGraphSync
depends on without retaining the schema.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Key pie.rows on the dataset instead of a JSON copy of it

The memo key serialized every row and column, so each dataset the user viewed
held roughly its own size in cache key for the life of the tab. A WeakMap on
the dataset with a short settings subkey keeps the same hit rate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Ban new uses of underscore's memoize

The 17 remaining call sites are allowlisted. The list may only shrink.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Narrow the memoize rule to module scope

A cache built inside a function, a component render, a class field or a
constructor is released with whatever owns it. Only a module scope cache lives
for the life of the tab, so the rule now flags just those.

This drops the allowlist from 17 files to 5, and each remaining entry carries
the reason its set of keys stays small.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Remove the memoize allowlist

Three of the five module scope caches are gone:

- getScrollBarSize takes no arguments, so it is a lazy value, not a cache.
- getScheduleExplanation returns a string that no caller compares by identity.
- cronToBuilderValue moves to the one caller that needs a stable identity.
  Schedule takes its value as a useMemo dependency, so StrategyForm now holds
  that memo itself. A user can write any number of cron strings, so a module
  cache for them was unbounded.

The remaining two carry an eslint-disable with the reason their set of keys
stays small.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Follow renderer_utils into the viz-core module

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Cover metabase/utils/memoize in the module scope rule

That memoize roots its WeakMap on the function it wraps, so it only releases
once that function does. At module scope the function never does, and the
nested Maps hold every argument ever passed. No call site does this today, but
the rule could not have caught one.

The rule now covers both sources, so it drops "underscore" from its name.
memoizeClass stays allowed: it keys on the instance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Drop re-reselect from the dashboard selectors

createCachedSelector keyed getQuestionByCard and
getDashcardParameterMappingOptions on the card id, a primitive, in a
FlatObjectCache. That cache is a plain object with no eviction, so a session
kept one Question per card id it ever saw, and each Question pins the whole
Metadata snapshot it was built with.

reselect v5 defaults to weakMapMemoize, which keys on the card and the metadata
objects themselves. Entries are released when their card leaves the store or
the metadata is replaced, and identity still holds for every card in the
working set, which is what connect and useSelector compare on.

Measured over 300 cards, with the states dropped: 6.2 MB retained before,
0.0 MB after. Per entry, 21462 B before and 51 B after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Drop re-reselect from the lockfile

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Back metabase/utils/memoize with reselect's weakMapMemoize

The hand-rolled implementation had two problems. It allocated two arrays on
every call, including cache hits: 225 ms against 20 ms for weakMapMemoize over
two million hits. And its WeakMap was keyed on the wrapped function rather than
the wrapper, so a cache built around a module-level helper outlived the wrapper
that owned it. A plain Map would have been safer.

weakMapMemoize matches on the same terms, identity for objects and value for
primitives, so no call site changes. It also weak-keys object arguments, which
the old one held strongly.

definition-cache passed weakMapMemoize as the memoize and argsMemoize options.
reselect v5 already defaults to both, so those are dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Route all memoization through metabase/utils/memoize

underscore's memoize is gone from the app. Its 16 call sites now use the house
helper, which is reselect's weakMapMemoize. Three of them were in metabase-lib,
which cannot import metabase/utils, and all three took no arguments, so they
became plain lazy fields instead.

Two rules keep it that way:

- no-direct-memoize-import bans memoize from underscore and weakMapMemoize or
  lruMemoize from reselect, in any scope. Only metabase/utils/memoize may reach
  for an implementation.
- no-module-level-memoize keeps its job, now that the house helper is the only
  one left to police.

measureTextWidthSafely took a style object that was rebuilt on every render, so
identity keying would never have hit. Its third argument is now the font family
string, which is the only field any caller ever set.

useMemoizedCallback returns the callable shape rather than casting to T, since
the memoized function also carries clearCache and resultsCount.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Build memoizeClass on the house memoize

It kept a module-level WeakMap and walked a nested Map trie keyed on the
instance, the method, the argument count and then each argument, assembling
that path into an array on every call.

Keying weakMapMemoize on the instance and then the arguments gives the same
per-instance caching. One cache per decorated method replaces the shared trie,
so the method no longer needs to be part of the key, and neither does the
argument count. The file is down to 61 lines from 128.

A leak spec covers the property this rests on: an instance's results are
released with the instance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Measure memoizeClass retention under load

2,000 instances per pass, 20 calls each, across a method with no arguments and
one taking a primitive and an object. Nothing is retained once the instances
are dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Delete memoizeClass

It decorated a prototype method by rewriting the property descriptor, and put
the memoization 600 lines away from the method it applied to. Its one consumer
memoized a method whose body uses no `this`, so the same caching comes from
assigning a memoized arrow to a class field. The field is created per instance,
so the cache is released with the instance, and the memoization is now visible
at the definition.

metabase/utils/memoize is 14 lines, from 128.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Run the heap specs from their own jest config

They were a project inside jest.config.js, and a bare `jest` runs every
project, which is what CI does. So the sharded unit run picked them up, without
--expose-gc, and they failed as designed.

They now live in jest.memory.conf.js, the way the timezone specs live in their
own config. The shared base moves to jest.base.conf.js so neither file has to
duplicate it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Reduce the heap specs to one collection assertion each

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

* Follow the store mocks into test support

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012b67ubbXJVMuQrpuEksbeh

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Romeo Van Snick
2026-09-09 15:02:13 +10:00
committed by GitHub
parent c1805286bb
commit e4cc4a61ff
51 changed files with 1419 additions and 593 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
bun.lock
View File

@@ -121,7 +121,6 @@
"prosemirror-state": "^1.4.4",
"prosemirror-view": "^1.40.1",
"querystring-es3": "^0.2.1",
"re-reselect": "^4.0.1",
"react": "^18.2.0",
"react-ansi-style": "^1.0.0",
"react-color": "^2.14.1",
@@ -4481,8 +4480,6 @@
"rc9": ["rc9@3.0.1", "", { "dependencies": { "defu": "^6.1.6", "destr": "^2.0.5" } }, "sha512-gMDyleLWVE+i6Sgtc0QbbY6pEKqYs97NGi6isHQPqYlLemPoO8dxQ3uGi0f4NiP98c+jMW6cG1Kx9dDwfvqARQ=="],
"re-reselect": ["re-reselect@4.0.1", "", {}, "sha512-xVTNGQy/dAxOolunBLmVMGZ49VUUR1s8jZUiJQb+g1sI63GAv9+a5Jas9yHvdxeUgiZkU9r3gDExDorxHzOgRA=="],
"react": ["react@18.2.0", "", { "dependencies": { "loose-envify": "^1.1.0" } }, "sha512-/3IjMdb2L9QbBdWiW5e3P2/npwMBaU9mHCSCUzNln0ZCYbcfTsGbTJrU/kGemdH2IWmB2ioZ+zkxtmq6g09fGQ=="],
"react-ansi-style": ["react-ansi-style@1.0.0", "", { "dependencies": { "ansi-style-parser": "^1.0.1", "js-managed-css": "^1.4.1" } }, "sha1-mx1Mju38VvTc81mjliGEZYw+jAk="],

47
enterprise/frontend/src/metabase-enterprise/schema_viewer/components/SchemaViewer/utils/flow-graph.leak.unit.spec.ts Normal file
View File

@@ -0,0 +1,47 @@
import { requireGarbageCollection, settleAndCollect } from "__support__/memory";
import type { ErdResponse } from "metabase-types/api";
import {
createMockErdField,
createMockErdNode,
createMockErdResponse,
} from "metabase-types/api/mocks";
import { toFlowGraph } from "./flow-graph";
const TABLES = 20;
const FIELDS_PER_TABLE = 10;
function makeSchema(): ErdResponse {
const nodes = Array.from({ length: TABLES }, (_, tableIndex) =>
createMockErdNode({
table_id: tableIndex,
name: `table_${tableIndex}`,
fields: Array.from({ length: FIELDS_PER_TABLE }, (_, fieldIndex) =>
createMockErdField({
id: tableIndex * FIELDS_PER_TABLE + fieldIndex,
name: `column_${fieldIndex}`,
}),
),
}),
);
return createMockErdResponse({ nodes });
}
/** Builds a graph and drops every reference to the response behind it. */
function makeGraphRef(): WeakRef<object> {
return new WeakRef(toFlowGraph(makeSchema()));
}
describe("toFlowGraph caching", () => {
it("releases a graph once its response is dropped", async () => {
requireGarbageCollection();
const graph = makeGraphRef();
await settleAndCollect();
// The memo key used to be a serialization of the whole schema, so every
// graph and its megabyte-scale key string outlived the response.
expect(graph.deref()).toBeUndefined();
});
});

72
enterprise/frontend/src/metabase-enterprise/schema_viewer/components/SchemaViewer/utils/flow-graph.ts
View File

@@ -1,5 +1,3 @@
import { memoize } from "underscore";
import * as Lib from "metabase-lib";
import type {
ErdEdge,
@@ -13,6 +11,11 @@ import type {
import { HEADER_HEIGHT_PX, NODE_WIDTH_PX, ROW_HEIGHT_PX } from "../constants";
import type { SchemaViewerFlowEdge, SchemaViewerFlowNode } from "../types";
type FlowGraph = {
nodes: SchemaViewerFlowNode[];
edges: SchemaViewerFlowEdge[];
};
export function getNodeId(node: { table_id: TableId } | TableId): string {
const tableId = typeof node === "object" ? node.table_id : node;
return `table-${tableId}`;
@@ -121,42 +124,7 @@ function toFlowEdge(edge: ErdEdge): SchemaViewerFlowEdge {
};
}
function getFlowGraphMemoKey(data: ErdResponse): string {
const nodeKey = data.nodes
.map((node) => {
const fieldKey = node.fields
.map(
(f) =>
`${f.id}:${f.name}:${f.display_name}:${f.database_type}:${f.base_type}:${f.effective_type ?? ""}:${f.semantic_type ?? ""}:${f.fk_target_field_id ?? ""}:${f.fk_target_table_id ?? ""}`,
)
.join("|");
return `${node.table_id}:${node.name}:${node.display_name}:${node.description ?? ""}:${getOwnerKey(node.owner)}:${node.schema ?? ""}:${node.visibility_type ?? ""}:${fieldKey}`;
})
.sort()
.join(";");
const edgeKey = data.edges
.map(
(edge) =>
`${edge.source_table_id}:${edge.source_field_id}->${edge.target_table_id}:${edge.target_field_id}:${edge.relationship}`,
)
.sort()
.join(";");
return `${nodeKey}__${edgeKey}`;
}
function getOwnerKey(owner: ErdNode["owner"]): string {
if (owner == null) {
return "";
}
if ("id" in owner && owner.id != null) {
return `u_${owner.id}`;
}
return `e_${owner.email}`;
}
const memoizedToFlowGraph = memoize((data: ErdResponse) => {
function buildFlowGraph(data: ErdResponse): FlowGraph {
// Per-table field roles: which fields act as source, target, or self-ref target
// of any edge. Handles rendering is based on this (not on semantic_type) so an
// edge whose target field isn't tagged as a PK still gets a matching handle.
@@ -190,11 +158,25 @@ const memoizedToFlowGraph = memoize((data: ErdResponse) => {
),
edges: data.edges.map((edge) => toFlowEdge(edge)),
};
}, getFlowGraphMemoKey);
export function toFlowGraph(data: ErdResponse): {
nodes: SchemaViewerFlowNode[];
edges: SchemaViewerFlowEdge[];
} {
return memoizedToFlowGraph(data);
}
/**
* Keyed on the response object, not on a serialization of the whole schema.
* RTK Query returns a stable reference per cache entry, so remounting the
* viewer still hits, and the entry is released when the query cache drops the
* response. Returning the identical graph matters beyond the compute it saves:
* useGraphSync takes the graph as an effect dependency, and a fresh object
* re-runs the layout and resets the viewport.
*/
const flowGraphs = new WeakMap<ErdResponse, FlowGraph>();
export function toFlowGraph(data: ErdResponse): FlowGraph {
const cached = flowGraphs.get(data);
if (cached !== undefined) {
return cached;
}
const graph = buildFlowGraph(data);
flowGraphs.set(data, graph);
return graph;
}

27
enterprise/frontend/src/metabase-enterprise/schema_viewer/components/SchemaViewer/utils/flow-graph.unit.spec.ts
View File

@@ -173,8 +173,27 @@ describe("toFlowGraph — per-table edge roles", () => {
});
});
describe("toFlowGraph — memoization", () => {
it("returns the same reference when called twice with deep-equal responses", () => {
describe("toFlowGraph — caching", () => {
// The cache is keyed on the response object. useGraphSync takes the graph as
// an effect dependency, so returning the identical graph for a response the
// viewer already holds is what stops the layout re-running and the viewport
// resetting. RTK Query keeps one reference per cache entry, so that holds
// across renders and remounts of the viewer.
it("returns the same reference for the same response object", () => {
const response: ErdResponse = {
nodes: [
createMockErdNode({
table_id: 1,
fields: [createMockErdField({ id: 10 })],
}),
],
edges: [createMockErdEdge({ source_field_id: 10, target_field_id: 20 })],
};
expect(toFlowGraph(response)).toBe(toFlowGraph(response));
});
it("returns a separate reference for a deep-equal but separate response", () => {
const responseA: ErdResponse = {
nodes: [
createMockErdNode({
@@ -186,9 +205,7 @@ describe("toFlowGraph — memoization", () => {
};
const responseB: ErdResponse = JSON.parse(JSON.stringify(responseA));
const a = toFlowGraph(responseA);
const b = toFlowGraph(responseB);
expect(a).toBe(b);
expect(toFlowGraph(responseA)).not.toBe(toFlowGraph(responseB));
});
it("returns a different reference when an edge's relationship changes", () => {

2
eslint.config.mjs
View File

@@ -332,6 +332,8 @@ const configs = [
"metabase/no-literal-metabase-strings": "error",
"metabase/no-oss-reinitialize-import": "error",
"metabase/no-analytics-import-outside-analytics-files": "error",
"metabase/no-module-level-memoize": "error",
"metabase/no-direct-memoize-import": "error",
"depend/ban-dependencies": [
"error",

2
frontend/lint/eslint-plugin-metabase/index.js
View File

@@ -25,5 +25,7 @@ module.exports = {
"no-analytics-import-outside-analytics-files": require("./rules/no-analytics-import-outside-analytics-files"),
"no-unjustified-type-casts": require("./rules/no-unjustified-type-casts"),
"valid-theme-tokens": require("./rules/valid-theme-tokens"),
"no-module-level-memoize": require("./rules/no-module-level-memoize"),
"no-direct-memoize-import": require("./rules/no-direct-memoize-import"),
},
};

91
frontend/lint/eslint-plugin-metabase/rules/no-direct-memoize-import.js Normal file
View File

@@ -0,0 +1,91 @@
/**
* @fileoverview Rule to keep memoization behind one helper, so there is a
* single implementation to reason about.
*/
const HOUSE_MEMOIZE = "metabase/utils/memoize";
const MESSAGE = [
"Import `memoize` from `metabase/utils/memoize` instead.",
"Keeping one helper means one set of semantics to reason about: it matches",
"object arguments by reference and primitives by value, and releases an entry",
"once its object arguments are gone.",
"underscore's memoize in particular never releases, and hashes only its first",
"argument unless you pass a hasher.",
].join(" ");
// Library exports that are memoize helpers in their own right.
const RESTRICTED = new Map([
["underscore", ["memoize"]],
["@reduxjs/toolkit", ["weakMapMemoize", "lruMemoize"]],
["reselect", ["weakMapMemoize", "lruMemoize"]],
]);
function isHouseMemoizeModule(filename) {
const normalized = filename.split("\\").join("/");
return normalized.endsWith(`${HOUSE_MEMOIZE}.ts`);
}
module.exports = {
meta: {
type: "problem",
docs: {
description: "Disallow importing memoize helpers straight from a library",
category: "Best Practices",
recommended: true,
},
schema: [],
messages: {
noDirectMemoizeImport: MESSAGE,
},
},
create(context) {
// The one module allowed to reach for the underlying implementation.
if (isHouseMemoizeModule(context.filename)) {
return {};
}
// Names bound to the underscore namespace, usually `_`.
const namespaceImports = new Set();
function report(node) {
context.report({ node, messageId: "noDirectMemoizeImport" });
}
return {
ImportDeclaration(node) {
const restricted = RESTRICTED.get(node.source.value);
if (restricted == null) {
return;
}
for (const specifier of node.specifiers) {
if (
specifier.type === "ImportSpecifier" &&
restricted.includes(specifier.imported.name)
) {
report(specifier);
} else if (
node.source.value === "underscore" &&
(specifier.type === "ImportDefaultSpecifier" ||
specifier.type === "ImportNamespaceSpecifier")
) {
// The namespace itself is fine, only `_.memoize` off it is not.
namespaceImports.add(specifier.local.name);
}
}
},
MemberExpression(node) {
if (
node.object.type === "Identifier" &&
namespaceImports.has(node.object.name) &&
node.property.type === "Identifier" &&
node.property.name === "memoize"
) {
report(node);
}
},
};
},
};

81
frontend/lint/eslint-plugin-metabase/rules/no-module-level-memoize.js Normal file
View File

@@ -0,0 +1,81 @@
/**
* @fileoverview Rule to prevent memoization at module scope, where the cache
* lives for the life of the tab.
*/
const HOUSE_MEMOIZE = "metabase/utils/memoize";
const MESSAGE = [
"A memoize built at module scope lives for the life of the tab.",
"It releases an entry once that entry's object arguments are gone, so a cache",
"keyed only on strings or numbers never releases at all.",
"Give the cache a lifetime that matches the work: build it inside the",
"function, component or instance that uses it, or key a WeakMap on the",
"long-lived object the result belongs to.",
].join(" ");
function isHouseMemoize(source, filename) {
if (source === HOUSE_MEMOIZE) {
return true;
}
// The helper's own neighbours import it relatively.
return (
/^\.{1,2}\/memoize$/.test(source) &&
filename.split("\\").join("/").includes("/metabase/utils/")
);
}
/**
* True when the call runs once, as the module is evaluated. A call inside a
* function, a component render, a class field or a constructor builds a fresh
* cache per call, which is released with whatever owns it.
*/
function runsAtModuleScope(scope) {
const { type } = scope.variableScope;
return type === "module" || type === "global";
}
module.exports = {
meta: {
type: "problem",
docs: {
description: "Disallow unbounded memoization at module scope",
category: "Best Practices",
recommended: true,
},
schema: [],
messages: {
noModuleLevelMemoize: MESSAGE,
},
},
create(context) {
const memoizeNames = new Set();
return {
ImportDeclaration(node) {
if (!isHouseMemoize(node.source.value, context.filename)) {
return;
}
for (const specifier of node.specifiers) {
if (
specifier.type === "ImportSpecifier" &&
specifier.imported.name === "memoize"
) {
memoizeNames.add(specifier.local.name);
}
}
},
"CallExpression > Identifier.callee"(node) {
if (
memoizeNames.has(node.name) &&
runsAtModuleScope(context.sourceCode.getScope(node))
) {
context.report({ node, messageId: "noModuleLevelMemoize" });
}
},
};
},
};

111
frontend/lint/tests/no-direct-memoize-import.unit.spec.js Normal file
View File

@@ -0,0 +1,111 @@
import { RuleTester } from "eslint";
import rule from "../eslint-plugin-metabase/rules/no-direct-memoize-import";
const ruleTester = new RuleTester({
languageOptions: { ecmaVersion: 2022, sourceType: "module" },
});
const error = { messageId: "noDirectMemoizeImport" };
const FILE = "/repo/frontend/src/metabase/thing/new-file.ts";
const HOUSE_MODULE = "/repo/frontend/src/metabase/utils/memoize.ts";
ruleTester.run("no-direct-memoize-import", rule, {
valid: [
{
name: "the house helper",
filename: FILE,
code: `
import { memoize } from "metabase/utils/memoize";
export const f = memoize((x) => x);
`,
},
{
name: "other underscore helpers",
filename: FILE,
code: `
import _ from "underscore";
export const picked = _.pick({ a: 1 }, "a");
`,
},
{
name: "other toolkit exports",
filename: FILE,
code: `
import { createSelector } from "@reduxjs/toolkit";
export const s = createSelector([(x) => x], (x) => x);
`,
},
{
name: "the house module may reach for the implementation",
filename: HOUSE_MODULE,
code: `export { weakMapMemoize as memoize } from "@reduxjs/toolkit";`,
},
{
name: "a memoize method on something else",
filename: FILE,
code: `
import cache from "./cache";
export const f = cache.memoize((x) => x);
`,
},
],
invalid: [
{
name: "underscore namespace access",
filename: FILE,
code: `
import _ from "underscore";
export const f = _.memoize((x) => x);
`,
errors: [error],
},
{
name: "underscore named import",
filename: FILE,
code: `
import { memoize } from "underscore";
export const f = memoize((x) => x);
`,
errors: [error],
},
{
name: "underscore renamed import",
filename: FILE,
code: `
import { memoize as underscoreMemoize } from "underscore";
export const f = underscoreMemoize((x) => x);
`,
errors: [error],
},
{
name: "weakMapMemoize from the toolkit",
filename: FILE,
code: `
import { weakMapMemoize } from "@reduxjs/toolkit";
export const f = weakMapMemoize((x) => x);
`,
errors: [error],
},
{
name: "lruMemoize from reselect",
filename: FILE,
code: `
import { lruMemoize } from "reselect";
export const f = lruMemoize((x) => x);
`,
errors: [error],
},
{
name: "underscore memoize used off module scope is still the wrong door",
filename: FILE,
code: `
import _ from "underscore";
export function build() {
return _.memoize((x) => x);
}
`,
errors: [error],
},
],
});

144
frontend/lint/tests/no-module-level-memoize.unit.spec.js Normal file
View File

@@ -0,0 +1,144 @@
import { RuleTester } from "eslint";
import rule from "../eslint-plugin-metabase/rules/no-module-level-memoize";
const ruleTester = new RuleTester({
languageOptions: { ecmaVersion: 2022, sourceType: "module" },
});
const error = { messageId: "noModuleLevelMemoize" };
const FILE = "/repo/frontend/src/metabase/thing/new-file.ts";
const UTIL_NEIGHBOUR = "/repo/frontend/src/metabase/utils/thing.ts";
const IMPORT = 'import { memoize } from "metabase/utils/memoize";';
ruleTester.run("no-module-level-memoize", rule, {
valid: [
{
name: "inside a plain function",
filename: FILE,
code: `
${IMPORT}
export function build(rows) {
const format = memoize((value) => String(value));
return rows.map(format);
}
`,
},
{
name: "inside a useMemo callback",
filename: FILE,
code: `
${IMPORT}
import { useMemo } from "react";
export function useThing(validate) {
return useMemo(() => ({ validate: memoize(validate) }), [validate]);
}
`,
},
{
name: "a class field initializer is per instance",
filename: FILE,
code: `
${IMPORT}
export class Question {
getParameters = memoize(() => []);
}
`,
},
{
name: "inside a constructor",
filename: FILE,
code: `
${IMPORT}
export class Table {
constructor() {
this.fieldsLookup = memoize(this.fieldsLookup);
}
}
`,
},
{
name: "inside an arrow returned by a factory",
filename: FILE,
code: `
${IMPORT}
export const createFormatter = () => memoize((value) => String(value));
`,
},
{
name: "memoizeClass at module scope is keyed on the instance",
filename: FILE,
code: `
import { memoizeClass } from "metabase/utils/memoize";
export const Wrapped = memoizeClass("render")(Thing);
`,
},
{
name: "an unrelated module that happens to be named memoize",
filename: "/repo/frontend/src/metabase/other/place.ts",
code: `
import { memoize } from "./memoize";
export const f = memoize((x) => x);
`,
},
{
name: "an unrelated local named memoize",
filename: FILE,
code: `
const memoize = (fn) => fn;
export const f = memoize((x) => x);
`,
},
],
invalid: [
{
name: "module scope",
filename: FILE,
code: `
${IMPORT}
export const f = memoize((x) => x);
`,
errors: [error],
},
{
name: "module scope with a renamed import",
filename: FILE,
code: `
import { memoize as cache } from "metabase/utils/memoize";
export const f = cache((x) => x);
`,
errors: [error],
},
{
name: "module scope inside an object literal",
filename: FILE,
code: `
${IMPORT}
export const definition = {
getValue: memoize((series) => series),
};
`,
errors: [error],
},
{
name: "imported relatively from inside utils",
filename: UTIL_NEIGHBOUR,
code: `
import { memoize } from "./memoize";
export const f = memoize((x) => x);
`,
errors: [error],
},
{
name: "every module scope call is reported",
filename: FILE,
code: `
${IMPORT}
export const f = memoize((x) => x);
export const g = memoize((y) => y);
`,
errors: [error, error],
},
],
});

17
frontend/src/metabase-lib/v1/Question.ts
View File

@@ -606,16 +606,17 @@ class Question {
return question;
}
private _getParameters = _.memoize(() => {
return getCardUiParameters(
this.card(),
this.metadata(),
this._parameterValues,
);
});
private _parameters?: ParameterObject[];
parameters(): ParameterObject[] {
return this._getParameters();
if (this._parameters == null) {
this._parameters = getCardUiParameters(
this.card(),
this.metadata(),
this._parameterValues,
);
}
return this._parameters;
}
// predicate function that determines if the question is "dirty" compared to the given question

15
frontend/src/metabase-lib/v1/metadata/Database.ts
View File

@@ -1,5 +1,3 @@
import _ from "underscore";
import { generateSchemaId } from "metabase-lib/v1/metadata/utils/schema";
import type { NativeQuery, NormalizedDatabase } from "metabase-types/api";
@@ -20,10 +18,10 @@ interface Database extends Omit<NormalizedDatabase, "tables" | "schemas"> {
*/
class Database {
private readonly _plainObject: NormalizedDatabase;
private _tablesLookup?: Record<string, Table>;
constructor(database: NormalizedDatabase) {
this._plainObject = database;
this.tablesLookup = _.memoize(this.tablesLookup);
Object.assign(this, database);
}
@@ -57,10 +55,17 @@ class Database {
return this.tables ?? [];
}
tablesLookup() {
return Object.fromEntries(
tablesLookup(): Record<string, Table> {
const cached = this._tablesLookup;
if (cached != null) {
return cached;
}
const lookup: Record<string, Table> = Object.fromEntries(
this.getTables().map((table) => [table.id, table]),
);
this._tablesLookup = lookup;
return lookup;
}
// @deprecated: use tablesLookup

15
frontend/src/metabase-lib/v1/metadata/Table.ts
View File

@@ -1,5 +1,3 @@
import _ from "underscore";
// NOTE: this needs to be imported first due to some cyclical dependency nonsense
import { singularize } from "metabase/utils/formatting";
import type { Measure, NormalizedTable, Segment } from "metabase-types/api";
@@ -32,10 +30,10 @@ interface Table extends Omit<
*/
class Table {
private readonly _plainObject: NormalizedTable;
private _fieldsLookup?: Record<string, Field>;
constructor(table: NormalizedTable) {
this._plainObject = table;
this.fieldsLookup = _.memoize(this.fieldsLookup);
Object.assign(this, table);
}
@@ -86,10 +84,17 @@ class Table {
}
// FIELDS
fieldsLookup() {
return Object.fromEntries(
fieldsLookup(): Record<string, Field> {
const cached = this._fieldsLookup;
if (cached != null) {
return cached;
}
const lookup: Record<string, Field> = Object.fromEntries(
this.getFields().map((field) => [field.id, field]),
);
this._fieldsLookup = lookup;
return lookup;
}
// @deprecated: use fieldsLookup

4
frontend/src/metabase/account/password/components/UserPasswordForm/UserPasswordForm.tsx
View File

@@ -1,6 +1,5 @@
import { useCallback, useMemo } from "react";
import { t } from "ttag";
import _ from "underscore";
import * as Yup from "yup";
import { useUpdatePasswordMutation } from "metabase/api";
@@ -13,6 +12,7 @@ import {
requiredErrorMessage,
} from "metabase/forms";
import { Group, Stack } from "metabase/ui";
import { memoize } from "metabase/utils/memoize";
import type { User } from "metabase-types/api";
import type { UserPasswordData } from "../../types";
@@ -47,7 +47,7 @@ export const UserPasswordForm = ({
}, []);
const validationContext = useMemo(
() => ({ onValidatePassword: _.memoize(onValidatePassword) }),
() => ({ onValidatePassword: memoize(onValidatePassword) }),
[onValidatePassword],
);

7
frontend/src/metabase/admin/performance/components/StrategyForm.tsx
View File

@@ -408,7 +408,12 @@ const ScheduleStrategyFormFields = ({
}) => {
const { values, setFieldValue } = useFormikContext<ScheduleStrategy>();
const { schedule: scheduleInCronFormat } = values;
const initialSchedule = cronToBuilderValue(scheduleInCronFormat);
// Schedule takes `value` as a useMemo dependency, so this has to keep its
// identity between renders for the same cron string.
const initialSchedule = useMemo(
() => cronToBuilderValue(scheduleInCronFormat),
[scheduleInCronFormat],
);
const timezone = useSelector((state) =>
getSetting(state, "report-timezone-short"),
);

4
frontend/src/metabase/auth/components/ResetPasswordForm/ResetPasswordForm.tsx
View File

@@ -1,6 +1,5 @@
import { useMemo } from "react";
import { t } from "ttag";
import _ from "underscore";
import * as Yup from "yup";
import {
@@ -12,6 +11,7 @@ import {
} from "metabase/forms";
import { Stack } from "metabase/ui";
import * as Errors from "metabase/utils/errors";
import { memoize } from "metabase/utils/memoize";
import { passwordComplexityDescription } from "metabase/utils/password";
import type { ResetPasswordData } from "../../types";
@@ -54,7 +54,7 @@ export const ResetPasswordForm = ({
}, []);
const validationContext = useMemo(
() => ({ onValidatePassword: _.memoize(onValidatePassword) }),
() => ({ onValidatePassword: memoize(onValidatePassword) }),
[onValidatePassword],
);

8
frontend/src/metabase/common/components/Schedule/AutoWidthSelect.tsx
View File

@@ -18,11 +18,11 @@ export const AutoWidthSelect = <Value extends string | null>({
getSetting(state, "application-font"),
);
const width = useMemo(() => {
const fontStyle = { family: fontFamily, ...style };
const longestLabel = getLongestSelectLabel(props.data, fontFamily);
const labelWidth = measureTextWidthSafely(longestLabel, 50, fontStyle);
const family = style?.family ?? fontFamily;
const longestLabel = getLongestSelectLabel(props.data, family);
const labelWidth = measureTextWidthSafely(longestLabel, 50, family);
const placeholderWidth = props.placeholder
? measureTextWidthSafely(props.placeholder, 50, fontStyle)
? measureTextWidthSafely(props.placeholder, 50, family)
: 0;
return Math.max(labelWidth, placeholderWidth) + 60;

5
frontend/src/metabase/common/components/Schedule/cron.ts
View File

@@ -1,5 +1,3 @@
import { memoize } from "underscore";
import type {
ScheduleDayType,
ScheduleFrameType,
@@ -109,7 +107,7 @@ export const scheduleValueToCron = (
return formatCron(value);
};
export const cronToBuilderValue_unmemoized = (
export const cronToBuilderValue = (
cron: string | null | undefined,
): ScheduleBuilderValue | null => {
if (!cron) {
@@ -194,7 +192,6 @@ export const cronToBuilderValue_unmemoized = (
schedule_frame,
};
};
export const cronToBuilderValue = memoize(cronToBuilderValue_unmemoized);
export const toScheduleBuilderValue = (
value: ScheduleValue,

28
frontend/src/metabase/common/components/Schedule/utils.tsx
View File

@@ -1,9 +1,8 @@
import type { ReactNode } from "react";
import _ from "underscore";
import type { SelectProps } from "metabase/ui";
import type { FontStyle } from "metabase/utils/measure-text";
import { measureTextWidth } from "metabase/utils/measure-text";
import { memoize } from "metabase/utils/memoize";
export const combineConsecutiveStrings = (arr: ReactNode[]) => {
return arr.reduce<ReactNode[]>((acc, node) => {
@@ -23,7 +22,7 @@ export const getLongestSelectLabel = (
fontFamily?: string,
): string => {
const width = (str: string) =>
measureTextWidthSafely(str, str.length, { family: fontFamily });
measureTextWidthSafely(str, str.length, fontFamily);
return [...data].reduce<string>((acc: string, option) => {
let label: string;
if (typeof option === "string") {
@@ -41,24 +40,27 @@ export const getLongestSelectLabel = (
}, "");
};
/** Since measureTextWidth can throw an error, this function catches the error and returns a default width
/**
* measureTextWidth can throw, so this returns defaultWidth instead.
*
* Note that you may want to set the style prop to reflect the currently chosen font family, like this:
* Pass the currently chosen font family:
* ```
* const fontFamily = useSelector(state => getSetting(state, "application-font"));
* measureTextWidthSafely("string", 50, {family: fontFamily});
* measureTextWidthSafely("string", 50, fontFamily);
* ```
* */
export const measureTextWidthSafely = _.memoize(
(text: string, defaultWidth: number, style?: Partial<FontStyle>) => {
*
* The arguments are all primitives on purpose. The cache matches them by value,
* and they come from a fixed set of schedule labels, so it cannot grow with
* anything the user does.
*/
// eslint-disable-next-line metabase/no-module-level-memoize
export const measureTextWidthSafely = memoize(
(text: string, defaultWidth: number, fontFamily?: string) => {
try {
return measureTextWidth(text, style);
return measureTextWidth(text, { family: fontFamily });
} catch (e) {
console.error(`Error while measuring text width:`, e);
return defaultWidth;
}
},
function hashFunction(...args) {
return JSON.stringify(args);
},
);

12
frontend/src/metabase/common/hooks/use-memoized-callback.ts
View File

@@ -5,12 +5,10 @@ import { memoize } from "metabase/utils/memoize";
export function useMemoizedCallback<T extends (...args: any[]) => any>(
callback: T,
deps: React.DependencyList = [],
): T {
return useMemo(() => {
// Unjustified type cast. FIXME
return memoize((...args: Parameters<T>): ReturnType<T> => {
return callback(...args);
}) as T;
): (...args: Parameters<T>) => ReturnType<T> {
return useMemo(
() => memoize((...args: Parameters<T>): ReturnType<T> => callback(...args)),
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps);
deps,
);
}

77
frontend/src/metabase/dashboard/selectors.leak.unit.spec.ts Normal file
View File

@@ -0,0 +1,77 @@
import { requireGarbageCollection, settleAndCollect } from "__support__/memory";
import { createMockSettingsState, createMockState } from "__support__/state";
import { createMockEntitiesState } from "__support__/store";
import { getQuestionByCard } from "metabase/dashboard/selectors";
import type { State } from "metabase/redux/store";
import type { Card } from "metabase-types/api";
import {
createMockCard,
createMockDatabase,
createMockField,
createMockTable,
} from "metabase-types/api/mocks";
const FIELDS_PER_TABLE = 60;
const TABLES = 40;
/** Metadata large enough that pinning one snapshot is visible on the heap. */
function makeState(cards: Card[]): State {
const tables = Array.from({ length: TABLES }, (_, tableIndex) =>
createMockTable({
id: tableIndex + 1,
db_id: 1,
name: `table_${tableIndex}`,
fields: Array.from({ length: FIELDS_PER_TABLE }, (_, fieldIndex) =>
createMockField({
id: tableIndex * FIELDS_PER_TABLE + fieldIndex + 1,
table_id: tableIndex + 1,
name: `column_${fieldIndex}_of_table_${tableIndex}`,
display_name: `Column ${fieldIndex} of table ${tableIndex}`,
}),
),
}),
);
return createMockState({
settings: createMockSettingsState(),
entities: createMockEntitiesState({
databases: [createMockDatabase({ id: 1, tables })],
tables,
questions: cards,
}),
});
}
/**
* Reads one card through a state of its own, so the Metadata it carries is
* reachable only through the selector cache once this returns.
*/
function makeMetadataRef(cardId: number): WeakRef<object> {
const card = createMockCard({ id: cardId, name: `Card ${cardId}` });
const state = makeState([card]);
const question = getQuestionByCard(state, { card });
if (question == null) {
throw new Error("expected a Question for a saved card");
}
return new WeakRef(question.metadata());
}
describe("getQuestionByCard caching", () => {
it("releases the metadata snapshot once its state is gone", async () => {
requireGarbageCollection();
const snapshots = [
makeMetadataRef(90_001),
makeMetadataRef(90_002),
makeMetadataRef(90_003),
];
// Read one more card so the most recent snapshot is no longer the current
// one. Memoization is expected to hold whatever it saw last.
makeMetadataRef(90_004);
await settleAndCollect();
// The cache keys weakly on the card and the metadata, so an entry cannot
// outlive them. Keying on the card id held one snapshot per generation.
expect(snapshots.filter((ref) => ref.deref() !== undefined)).toEqual([]);
});
});

20
frontend/src/metabase/dashboard/selectors.ts
View File

@@ -1,5 +1,4 @@
import { createSelector } from "@reduxjs/toolkit";
import { createCachedSelector } from "re-reselect";
import _ from "underscore";
import { LOAD_COMPLETE_FAVICON } from "metabase/common/hooks/constants";
@@ -490,9 +489,13 @@ export const getMissingRequiredParameters = createSelector(
);
/**
* It's a memoized version, it uses LRU cache per card identified by id
* Holds one Question per card, so that connected components keep the same
* reference between renders. reselect keys weakly on the card and the metadata,
* so an entry is released once its card leaves the store or the metadata is
* replaced. Keying on the card id instead would hold every Question, and the
* metadata snapshot each one carries, for the life of the tab.
*/
export const getQuestionByCard = createCachedSelector(
export const getQuestionByCard = createSelector(
[
(_state: State, props: { card: Card | VirtualCard }) => props.card,
getMetadata,
@@ -500,12 +503,9 @@ export const getQuestionByCard = createCachedSelector(
(card, metadata) => {
return isQuestionCard(card) ? new Question(card, metadata) : undefined;
},
)((_state, props) => {
// Virtual cards don't have an ID and should not return a question so we use "virtual" as a cache key for all of them
return props.card.id == null ? "virtual" : props.card.id;
});
);
export const getDashcardParameterMappingOptions = createCachedSelector(
export const getDashcardParameterMappingOptions = createSelector(
[getQuestionByCard, getEditingParameter, getCard, getDashCard, getDashcards],
(question, parameter, card, dashcard, dashcards) => {
const parameterDashcard =
@@ -520,9 +520,7 @@ export const getDashcardParameterMappingOptions = createCachedSelector(
parameterDashcard,
);
},
)((state, props) => {
return props.card.id ?? props.dashcard.id;
});
);
// Embeddings might be published without passing embedding_params to the server,
// in which case it's an empty object. We should treat such situations with

48
frontend/src/metabase/dashboard/selectors.unit.spec.ts
View File

@@ -15,22 +15,26 @@ import {
getIsEditingParameter,
getIsSharing,
getParameters,
getQuestionByCard,
getSelectedTabId,
getShowAddQuestionSidebar,
getSidebar,
} from "metabase/dashboard/selectors";
import type { State } from "metabase/redux/store";
import Field from "metabase-lib/v1/metadata/Field";
import type { Card } from "metabase-types/api";
import {
createMockCard,
createMockDashboard,
createMockDashboardCard,
createMockDashboardTab,
createMockDatabase,
createMockField,
createMockHeadingDashboardCard,
createMockNativeDatasetQuery,
createMockParameter,
createMockStructuredDatasetQuery,
createMockTable,
} from "metabase-types/api/mocks";
import { SIDEBAR_NAME } from "./constants";
@@ -519,3 +523,47 @@ describe("getSelectedTabId", () => {
expect(getSelectedTabId(state)).toBe(1);
});
});
describe("getQuestionByCard", () => {
function makeState(cards: Card[]): State {
const table = createMockTable({
id: 1,
db_id: 1,
fields: [createMockField({ id: 1, table_id: 1 })],
});
return createMockState({
settings: createMockSettingsState(),
entities: createMockEntitiesState({
databases: [createMockDatabase({ id: 1, tables: [table] })],
tables: [table],
questions: cards,
}),
});
}
it("returns the identical Question for the same card and state", () => {
const card = createMockCard({ id: 1 });
const state = makeState([card]);
// connect() shallow-compares mapped props, so a fresh Question here would
// re-render DashCardCardParameterMapper on every store change.
expect(getQuestionByCard(state, { card })).toBe(
getQuestionByCard(state, { card }),
);
});
it("holds an entry per card rather than only the most recent one", () => {
const cards = [1, 2, 3].map((id) => createMockCard({ id }));
const state = makeState(cards);
const first = cards.map((card) => getQuestionByCard(state, { card }));
// Reading the other cards in between must not evict the first. A one-entry
// cache would recompute here and re-render every mapped dashcard.
const second = cards.map((card) => getQuestionByCard(state, { card }));
first.forEach((question, index) => {
expect(question).toBe(second[index]);
});
});
});

4
frontend/src/metabase/databases/components/DatabaseForm/DatabaseForm.tsx
View File

@@ -1,8 +1,8 @@
import { useCallback, useMemo } from "react";
import _ from "underscore";
import { Form, FormProvider } from "metabase/forms";
import { useSetting } from "metabase/settings";
import { memoize } from "metabase/utils/memoize";
import type { DatabaseData } from "metabase-types/api";
import type { FormLocation } from "../../types";
@@ -69,7 +69,7 @@ export const DatabaseForm = ({
}, [engines, initialData, isAdvanced]);
const getSchema = useMemo(() => {
return _.memoize((engineKey: string | undefined) =>
return memoize((engineKey: string | undefined) =>
getValidationSchema(getEngine(engines, engineKey), engineKey, isAdvanced),
);
}, [engines, isAdvanced]);

10
frontend/src/metabase/metrics-viewer/utils/definition-cache.ts
View File

@@ -1,4 +1,4 @@
import { createSelector, weakMapMemoize } from "@reduxjs/toolkit";
import { createSelector } from "@reduxjs/toolkit";
import * as LibMetric from "metabase-lib/metric";
import type { JsMetricDefinition } from "metabase-types/api";
@@ -83,10 +83,6 @@ export const getModifiedDefinition = createSelector(
return executableDefinition;
},
{
memoize: weakMapMemoize,
argsMemoize: weakMapMemoize,
},
);
export const toJsDefinition = createSelector(
@@ -94,8 +90,4 @@ export const toJsDefinition = createSelector(
(definition): JsMetricDefinition => {
return LibMetric.toJsMetricDefinition(definition);
},
{
memoize: weakMapMemoize,
argsMemoize: weakMapMemoize,
},
);

25
frontend/src/metabase/monitor/components/LogsViewer/utils.ts
View File

@@ -1,5 +1,3 @@
import _ from "underscore";
import { dayjs } from "metabase/dayjs";
import type { Log } from "metabase-types/api";
@@ -9,14 +7,29 @@ export function getAllProcessUUIDs(logs: Log[]) {
return [...uuids].filter(Boolean).sort();
}
// date formatting is expensive for megabytes of logs
const formatTs = (ts: string) => dayjs(ts).format();
const memoedFormatTs = _.memoize(formatTs);
/**
* Date formatting is expensive for megabytes of logs, and the polling viewer
* reformats the whole accumulated list on every render and every filter
* keystroke. Keying on the log object rather than on its timestamp lets an
* entry die with the log instead of living as long as the tab.
*/
const formattedTimestamps = new WeakMap<Log, string>();
function formatTimestamp(log: Log): string {
const cached = formattedTimestamps.get(log);
if (cached !== undefined) {
return cached;
}
const formatted = dayjs(log.timestamp).format();
formattedTimestamps.set(log, formatted);
return formatted;
}
export const createLogFormatter =
(process: string, processUUIDs: string[]) =>
(log: Log): string[] => {
const timestamp = memoedFormatTs(log.timestamp);
const timestamp = formatTimestamp(log);
const uuid =
process === "ALL" && processUUIDs.length > 1
? `[${log.process_uuid}]`

37
frontend/src/metabase/monitor/components/LogsViewer/utils.unit.spec.ts Normal file
View File

@@ -0,0 +1,37 @@
import { createLogFormatter } from "metabase/monitor/components/LogsViewer/utils";
import type { Log } from "metabase-types/api";
const PROCESS_UUID = "3f2a1c00-0000-4000-8000-000000000000";
function makeLog(timestampMs: number): Log {
return {
timestamp: new Date(timestampMs).toISOString(),
process_uuid: PROCESS_UUID,
fqns: "metabase.server.middleware.log",
msg: "GET /api/dashboard/1 200",
level: "INFO",
exception: null,
};
}
describe("createLogFormatter", () => {
it("reuses the formatted timestamp for the same log object", () => {
const log = makeLog(Date.UTC(2024, 0, 1));
const formatLog = createLogFormatter("ALL", [PROCESS_UUID]);
const firstLine = formatLog(log)[0];
// The app never mutates a log. This is the only way to observe that the
// second call read the cache instead of reformatting.
log.timestamp = new Date(Date.UTC(2024, 6, 1)).toISOString();
expect(formatLog(log)[0]).toBe(firstLine);
});
it("formats a separate log object independently", () => {
const formatLog = createLogFormatter("ALL", [PROCESS_UUID]);
expect(formatLog(makeLog(Date.UTC(2024, 0, 1)))[0]).not.toBe(
formatLog(makeLog(Date.UTC(2024, 6, 1)))[0],
);
});
});

39
frontend/src/metabase/querying/expressions/clause.ts
View File

@@ -1,6 +1,5 @@
import _ from "underscore";
import { hasRequiredFeature } from "metabase/databases";
import { memoize } from "metabase/utils/memoize";
import { isNotNull } from "metabase/utils/types";
import type * as Lib from "metabase-lib";
import {
@@ -60,25 +59,25 @@ export function getMBQLName(
return EXPRESSION_TO_MBQL_NAME.get(expressionName.trim().toLowerCase());
}
export const clausesForMode = _.memoize(
(expressionMode: Lib.ExpressionMode) => {
const base =
expressionMode === "aggregation"
? AGGREGATION_FUNCTIONS
: EXPRESSION_FUNCTIONS;
// The cache is keyed on the expression mode, of which there are four, so it
// cannot grow with anything the user does.
// eslint-disable-next-line metabase/no-module-level-memoize
export const clausesForMode = memoize((expressionMode: Lib.ExpressionMode) => {
const base =
expressionMode === "aggregation"
? AGGREGATION_FUNCTIONS
: EXPRESSION_FUNCTIONS;
return Object.keys(base)
.map(getClauseDefinition)
.filter(isNotNull)
.filter(function excludeOffsetInFilterExpressions(clause) {
const isOffset = clause.name === "offset";
const isFilterExpression = expressionMode === "filter";
return !isOffset || !isFilterExpression;
})
.sort((a, b) => a.name.localeCompare(b.name));
},
(expressionMode) => expressionMode,
);
return Object.keys(base)
.map(getClauseDefinition)
.filter(isNotNull)
.filter(function excludeOffsetInFilterExpressions(clause) {
const isOffset = clause.name === "offset";
const isFilterExpression = expressionMode === "filter";
return !isOffset || !isFilterExpression;
})
.sort((a, b) => a.name.localeCompare(b.name));
});
export function getSupportedClauses({
expressionMode,

8
frontend/src/metabase/querying/expressions/resolver.ts
View File

@@ -1,6 +1,6 @@
import { c, t } from "ttag";
import _ from "underscore";
import { memoize } from "metabase/utils/memoize";
import * as Lib from "metabase-lib";
import { CompileError } from "./errors";
@@ -30,11 +30,11 @@ export function resolver(options: Options): Resolver {
availableMetrics,
} = options;
const metrics = _.memoize(
const metrics = memoize(
() => availableMetrics ?? Lib.availableMetrics(query, stageIndex),
);
const segments = _.memoize(() => Lib.availableSegments(query, stageIndex));
const measures = _.memoize(() => Lib.availableMeasures(query, stageIndex));
const segments = memoize(() => Lib.availableSegments(query, stageIndex));
const measures = memoize(() => Lib.availableMeasures(query, stageIndex));
const cache = infoCache(options);

4
frontend/src/metabase/querying/notebook/components/FilterStep/FilterStep.tsx
View File

@@ -1,12 +1,12 @@
import { useMemo } from "react";
import { t } from "ttag";
import _ from "underscore";
import ErrorBoundary from "metabase/ErrorBoundary";
import { useLocale } from "metabase/common/hooks";
import { useTranslateContent } from "metabase/content-translation/hooks";
import { FilterPicker } from "metabase/querying/filters/components/FilterPicker";
import { getTranslatedFilterDisplayName } from "metabase/querying/filters/utils/display";
import { memoize } from "metabase/utils/memoize";
import * as Lib from "metabase-lib";
import type { NotebookStepProps } from "../../types";
@@ -31,7 +31,7 @@ export function FilterStep({
const renderFilterName = useMemo(
() =>
_.memoize((filter: Lib.FilterClause) =>
memoize((filter: Lib.FilterClause) =>
getTranslatedFilterDisplayName(query, stageIndex, filter, tc, locale),
),
[query, stageIndex, tc, locale],

4
frontend/src/metabase/segments/components/SegmentFilterEditor/SegmentFilterEditor.tsx
View File

@@ -1,6 +1,5 @@
import { useCallback, useMemo } from "react";
import { t } from "ttag";
import _ from "underscore";
import ErrorBoundary from "metabase/ErrorBoundary";
import { useLocale } from "metabase/common/hooks";
@@ -8,6 +7,7 @@ import { useTranslateContent } from "metabase/content-translation/hooks";
import { FilterPicker } from "metabase/querying/filters/components/FilterPicker";
import { getTranslatedFilterDisplayName } from "metabase/querying/filters/utils/display";
import { ClauseStep } from "metabase/querying/notebook/components/ClauseStep";
import { memoize } from "metabase/utils/memoize";
import * as Lib from "metabase-lib";
const STAGE_INDEX = -1;
@@ -29,7 +29,7 @@ export function SegmentFilterEditor({
const renderFilterName = useMemo(
() =>
_.memoize((filter: Lib.FilterClause) =>
memoize((filter: Lib.FilterClause) =>
getTranslatedFilterDisplayName(query, STAGE_INDEX, filter, tc, locale),
),
[query, tc, locale],

4
frontend/src/metabase/setup/components/UserForm/UserForm.tsx
View File

@@ -1,6 +1,5 @@
import { useMemo } from "react";
import { t } from "ttag";
import _ from "underscore";
import * as Yup from "yup";
import { useValidatePassword } from "metabase/common/hooks";
@@ -13,6 +12,7 @@ import {
import type { UserInfo } from "metabase/redux/store";
import { Flex, Stack } from "metabase/ui";
import * as Errors from "metabase/utils/errors";
import { memoize } from "metabase/utils/memoize";
import { UserFieldGroup } from "./UserForm.styled";
@@ -44,7 +44,7 @@ interface UserFormProps {
export const UserForm = ({ user, isHosted, onSubmit }: UserFormProps) => {
const validatePassword = useValidatePassword();
const validationContext = useMemo(
() => ({ onValidatePassword: _.memoize(validatePassword) }),
() => ({ onValidatePassword: memoize(validatePassword) }),
[validatePassword],
);
const initialValues = useMemo(() => {

20
frontend/src/metabase/utils/cron.ts
View File

@@ -1,7 +1,6 @@
import { isValidCronExpression } from "cron-expression-validator";
import cronstrue from "cronstrue";
import { t } from "ttag";
import { memoize } from "underscore";
import MetabaseSettings from "metabase/utils/settings";
import { has24HourModeSetting } from "metabase/utils/time-dayjs";
@@ -73,18 +72,13 @@ function lowerCaseFirstLetter(str: string) {
return str.charAt(0).toLowerCase() + str.slice(1);
}
export const getScheduleExplanation = memoize(
(cronExpression: string): string | null => {
try {
const readableSchedule = lowerCaseFirstLetter(
explainCronExpression(cronExpression),
);
return readableSchedule;
} catch {
return null;
}
},
);
export function getScheduleExplanation(cronExpression: string): string | null {
try {
return lowerCaseFirstLetter(explainCronExpression(cronExpression));
} catch {
return null;
}
}
// Remove seconds and years
export function formatCronExpressionForUI(cronExpression: string): string {

19
frontend/src/metabase/utils/dom.ts
View File

@@ -1,12 +1,7 @@
import _ from "underscore";
import { isWithinIframe } from "metabase/utils/iframe";
import MetabaseSettings from "metabase/utils/settings";
// check whether scrollbars are visible to the user,
// this is off by default on Macs, but can be changed
// Always on on most other non mobile platforms
export const getScrollBarSize = _.memoize((): number => {
function measureScrollBarSize(): number {
const scrollableElem = document.createElement("div"),
innerElem = document.createElement("div");
scrollableElem.style.width = "30px";
@@ -20,7 +15,17 @@ export const getScrollBarSize = _.memoize((): number => {
const diff = scrollableElem.offsetWidth - scrollableElem.clientWidth;
document.body.removeChild(scrollableElem);
return diff;
});
}
let scrollBarSize: number | undefined;
// check whether scrollbars are visible to the user,
// this is off by default on Macs, but can be changed
// Always on on most other non mobile platforms
export function getScrollBarSize(): number {
scrollBarSize ??= measureScrollBarSize();
return scrollBarSize;
}
// check if we have access to localStorage to avoid handling "access denied"
// exceptions

24
frontend/src/metabase/utils/memoize.leak.unit.spec.ts Normal file
View File

@@ -0,0 +1,24 @@
import { requireGarbageCollection, settleAndCollect } from "__support__/memory";
import { memoize } from "metabase/utils/memoize";
/** The pattern that replaced memoizeClass: a memoized arrow on a class field. */
class Chart {
series = memoize(() => ({ rows: [1, 2, 3] }));
}
function makeResultRef(): WeakRef<object> {
return new WeakRef(new Chart().series());
}
describe("a memoized class field", () => {
it("releases an instance's results with the instance", async () => {
requireGarbageCollection();
const result = makeResultRef();
await settleAndCollect();
// The field is created per instance, so nothing outlives it. A cache built
// once on the prototype would hold every instance's results instead.
expect(result.deref()).toBeUndefined();
});
});

124
frontend/src/metabase/utils/memoize.ts
View File

@@ -1,118 +1,14 @@
type Constructor<T> = new (...args: any[]) => T;
type MapLike<K, V> = Map<K, V> | WeakMap<WeakKey, V>;
function getMapLikeWithFallback<K, V>(
map: MapLike<K, V>,
key: K,
fallback: () => V,
): V {
// Unjustified type cast. FIXME
if ("has" in map && map.has(key as any)) {
// Unjustified type cast. FIXME
return map.get(key as any)!;
} else {
const value = fallback();
// Unjustified type cast. FIXME
map.set(key as any, value);
return value;
}
}
type CacheValue = unknown;
type CacheMap = Map<unknown, CacheValue | CacheMap>;
const rootCache = new WeakMap<object, CacheMap>();
const createCacheMap = (): CacheMap => new Map();
export function memoize<T extends (...args: any[]) => any>(fn: T): T {
const memoizedFn = (...args: Parameters<T>): ReturnType<T> => {
const [first, ...rest] = [fn, args.length, ...args];
const lastKey = rest.pop()!;
let currentMap = getMapLikeWithFallback(rootCache, first, createCacheMap);
for (const key of rest) {
const nextMap = getMapLikeWithFallback(currentMap, key, createCacheMap);
if (nextMap instanceof Map) {
currentMap = nextMap;
} else {
throw new Error("Invalid cache structure");
}
}
return getMapLikeWithFallback(currentMap, lastKey, () => fn(...args));
};
// Unjustified type cast. FIXME
return memoizedFn as T;
}
function getWithFallback(
map: Map<string, any>,
key: string,
fallback: () => void,
) {
if (map.has(key)) {
return map.get(key);
} else {
const value = fallback();
map.set(key, value);
return value;
}
}
const memoized = new WeakMap();
const createMap = () => new Map();
/**
* This method implements memoization for class methods
* It creates a map where class itself, method and all the parameters are used as keys for a nested map
* map<class, map<method, map<param1, map<param2, map<param3...>>>>>
* The single memoize helper for the app. Import it from here rather than from
* underscore or reselect, so there is one implementation to reason about.
*
* If you use objects as parameters, make sure their references are stable as they will be used as keys
* It keys on every argument: object arguments by reference through a WeakMap,
* primitives by value through a Map. An entry with an object argument is
* released once that object is. An entry keyed only on primitives is not, so do
* not build one of these at module scope, where it lives for the life of the
* tab. See the no-module-level-memoize lint rule.
*
* @param keys - class methods to memoize
* @returns the same class with memoized methods
* To memoize a class method, assign a memoized arrow to a class field. The
* field is created per instance, so its cache is released with the instance.
*/
export function memoizeClass<T>(
...keys: string[]
): (Class: Constructor<T>) => Constructor<T> {
return (Class: Constructor<T>): Constructor<T> => {
const descriptors = Object.getOwnPropertyDescriptors(Class.prototype);
keys.forEach((key) => {
// Is targeted method present in Class?
if (!(key in descriptors)) {
throw new TypeError(`${key} is not a member of class`);
}
const descriptor = descriptors[key];
const method = descriptor.value;
// If we don't get a descriptor.value, it must have a getter (i.e., ES6 class properties)
if (!method) {
throw new TypeError(`Class properties cannot be memoized`);
}
// Method should be a function/method
else if (typeof method !== "function") {
throw new TypeError(`${key} is not a method and cannot be memoized`);
}
// Memoize
Object.defineProperty(Class.prototype, key, {
...descriptor,
value: function (...args: any[]) {
const path = [this, method, args.length, ...args];
const last = path.pop();
const map = path.reduce(
(map, key) => getWithFallback(map, key, createMap),
memoized,
);
return getWithFallback(map, last, () => method.apply(this, args));
},
});
});
return Class;
};
}
export { weakMapMemoize as memoize } from "@reduxjs/toolkit";

95
frontend/src/metabase/utils/memoize.unit.spec.ts
View File

@@ -1,78 +1,33 @@
import { memoizeClass } from "./memoize";
import { memoize } from "metabase/utils/memoize";
describe("memoize", () => {
it("should memoize method", () => {
let x = 0;
class fooInner {
bar() {
return ++x;
}
}
const foo = memoizeClass<fooInner>("bar")(fooInner);
const f = new foo();
expect(f.bar()).toEqual(1);
expect(f.bar()).toEqual(1);
class Chart {
series = memoize(() => ({ rows: [1, 2, 3] }));
slice = memoize((index: number, options: { label: string }) => ({
index,
label: options.label,
}));
}
describe("a memoized class field", () => {
it("returns the identical result for one instance", () => {
const chart = new Chart();
expect(chart.series()).toBe(chart.series());
});
it("should memoize method with objects", () => {
class fooInner {
bar() {
return {};
}
}
const foo = memoizeClass<fooInner>("bar")(fooInner);
const f = new foo();
const x = f.bar();
expect(f.bar()).toEqual(x);
it("keeps instances apart", () => {
expect(new Chart().series()).not.toBe(new Chart().series());
});
it("should use args in cache key", () => {
class fooInner {
bar(a: number, b: number, c: number) {
return a + b + c;
}
}
const foo = memoizeClass<fooInner>("bar")(fooInner);
const f = new foo();
expect(f.bar(1, 2, 3)).toEqual(6);
expect(f.bar(1, 2, 4)).toEqual(7);
});
it("keys on every argument", () => {
const chart = new Chart();
const options = { label: "first" };
it("should allow calling with variable number of args", () => {
class fooInner {
bar(x?: number) {
return x;
}
}
const foo = memoizeClass<fooInner>("bar")(fooInner);
const f = new foo();
expect(f.bar()).toEqual(undefined);
expect(f.bar(1)).toEqual(1);
});
it("should memoize multiple methods", () => {
let x = 0;
class fooInner {
bar() {
return ++x;
}
biz() {
return ++x;
}
}
const foo = memoizeClass<fooInner>("bar", "biz")(fooInner);
const f = new foo();
expect(f.bar()).toEqual(1);
expect(f.bar()).toEqual(1);
expect(f.biz()).toEqual(2);
expect(f.biz()).toEqual(2);
});
it("should throw on nonexistent keys", () => {
expect(() => {
class fooInner {}
memoizeClass("bar")(fooInner);
}).toThrow();
expect(chart.slice(1, options)).toBe(chart.slice(1, options));
expect(chart.slice(1, options)).not.toBe(chart.slice(2, options));
expect(chart.slice(1, options)).not.toBe(
chart.slice(1, { label: "first" }),
);
});
});

96
frontend/src/metabase/visualizations/components/Visualization/Visualization.tsx
View File

@@ -29,7 +29,7 @@ import { getFont } from "metabase/styled-components/selectors";
import type { IconProps } from "metabase/ui";
import { isQuestionCard } from "metabase/utils/dashboard";
import { formatNumber } from "metabase/utils/formatting";
import { memoizeClass } from "metabase/utils/memoize";
import { memoize } from "metabase/utils/memoize";
import { getVisualizationComponent } from "metabase/visualizations";
import ChartCaption from "metabase/visualizations/components/ChartCaption";
import ChartTooltip from "metabase/visualizations/components/ChartTooltip";
@@ -437,52 +437,56 @@ class Visualization extends PureComponent<
return !!card && !!metadata ? new Question(card, metadata) : undefined;
}
_getClickActionsCached(
clickedObject: ClickObject | null | undefined,
mode: ClickActionsMode | undefined,
computedSettings: Record<string, string>,
dashcard?: DashboardCard,
metadata?: Metadata,
rawSeries: VisualizationRawSeries = [],
visualizerRawSeries: RawSeries = [],
isRawTable = false,
getExtraDataForClick: (
clicked: ClickObject | null,
) => Record<string, unknown> = () => ({}),
transformClickObject?: (clicked: ClickObject) => ClickObject,
) {
if (!clickedObject) {
return [];
}
// Memoized per instance. The cache keys on the arguments, and the object
// ones are held weakly, so entries go when the click context does.
private _getClickActionsCached = memoize(
(
clickedObject: ClickObject | null | undefined,
mode: ClickActionsMode | undefined,
computedSettings: Record<string, string>,
dashcard?: DashboardCard,
metadata?: Metadata,
rawSeries: VisualizationRawSeries = [],
visualizerRawSeries: RawSeries = [],
isRawTable = false,
getExtraDataForClick: (
clicked: ClickObject | null,
) => Record<string, unknown> = () => ({}),
transformClickObject?: (clicked: ClickObject) => ClickObject,
) => {
if (!clickedObject) {
return [];
}
const clicked = transformClickObject
? transformClickObject(clickedObject)
: clickedObject;
const clicked = transformClickObject
? transformClickObject(clickedObject)
: clickedObject;
const card = Visualization.findCardById(
clicked.cardId,
dashcard,
rawSeries,
visualizerRawSeries,
);
if (!isQuestionCard(card)) {
return [];
}
const question = Visualization.getQuestionForCard(metadata, card);
const card = Visualization.findCardById(
clicked.cardId,
dashcard,
rawSeries,
visualizerRawSeries,
);
if (!isQuestionCard(card)) {
return [];
}
const question = Visualization.getQuestionForCard(metadata, card);
return mode
? mode.actionsForClick(
{
...clicked,
extraData: {
...getExtraDataForClick(clicked),
isRawTable,
return mode
? mode.actionsForClick(
{
...clicked,
extraData: {
...getExtraDataForClick(clicked),
isRawTable,
},
},
},
{ question, settings: computedSettings },
)
: [];
}
{ question, settings: computedSettings },
)
: [];
},
);
getClickActions(clickedObject?: ClickObject | null) {
const {
@@ -1032,10 +1036,6 @@ class Visualization extends PureComponent<
}
}
const VisualizationMemoized = memoizeClass<Visualization>(
"_getClickActionsCached",
)(Visualization);
// eslint-disable-next-line import/no-default-export
export default _.compose(
connect(mapStateToProps),
@@ -1068,7 +1068,7 @@ export default _.compose(
return <VisualizationRunningState className={cx(CS.spread, CS.z2)} />;
}
return <VisualizationMemoized {...props} forwardedRef={ref} />;
return <Visualization {...props} forwardedRef={ref} />;
},
),
) as ComponentType<VisualizationOwnProps>;

7
frontend/src/metabase/visualizations/lib/data_grid.ts
View File

@@ -1,6 +1,7 @@
import _ from "underscore";
import * as Pivot from "cljs/metabase.pivot.js";
import { memoize } from "metabase/utils/memoize";
import { checkNotNull } from "metabase/utils/types";
import { formatValue } from "metabase/value-formatting";
import type {
@@ -97,10 +98,8 @@ export function multiLevelPivot(
rowIndexes,
].map((indexes) =>
indexes.map((index) =>
_.memoize(
(value: RowValue) => formatValue(value, columnSettings[index]),
(value: RowValue) => JSON.stringify(value) + String(index),
),
// `index` is closed over, so the value is the whole key.
memoize((value: RowValue) => formatValue(value, columnSettings[index])),
),
);

65
frontend/src/metabase/visualizations/visualizations/PieChart/definition.leak.unit.spec.ts Normal file
View File

@@ -0,0 +1,65 @@
import { requireGarbageCollection, settleAndCollect } from "__support__/memory";
import { PIE_CHART_DEFINITION } from "metabase/visualizations/visualizations/PieChart/definition";
import type { RawSeries, RowValues } from "metabase-types/api";
import {
createMockCard,
createMockColumn,
createMockDatasetData,
} from "metabase-types/api/mocks";
const ROWS = 200;
/**
* `pie.metric` and `pie.dimension` are absent, so getPieRows returns early.
* That keeps this test about the cache rather than about the row computation.
*/
const NO_SETTINGS = {};
function makeSeries(): RawSeries {
const rows: RowValues[] = Array.from({ length: ROWS }, (_, index) => [
`category ${index}`,
index,
]);
return [
{
card: createMockCard(),
data: createMockDatasetData({
rows,
cols: [
createMockColumn({ name: "category", display_name: "Category" }),
createMockColumn({ name: "count", display_name: "Count" }),
],
}),
},
];
}
/** Computes the rows for one dataset, then drops every reference to it. */
function makeRowsRef(): WeakRef<object> {
const getValue = PIE_CHART_DEFINITION.settings?.["pie.rows"]?.getValue;
if (getValue == null) {
throw new Error(
"PIE_CHART_DEFINITION no longer exposes a pie.rows getValue",
);
}
const rows = getValue(makeSeries(), NO_SETTINGS);
if (!Array.isArray(rows)) {
throw new Error("expected pie.rows to compute an array");
}
return new WeakRef(rows);
}
describe("PieChart pie.rows caching", () => {
it("releases the computed rows once the chart is gone", async () => {
requireGarbageCollection();
const rows = makeRowsRef();
await settleAndCollect();
// The cache used to live at module scope, keyed on a JSON copy of every row
// and column, so each result and its key outlived the chart.
expect(rows.deref()).toBeUndefined();
});
});

58
frontend/src/metabase/visualizations/visualizations/PieChart/definition.ts
View File

@@ -25,10 +25,20 @@ import {
nestedSettings,
} from "metabase/viz-core";
import { isDimension, isMetric } from "metabase-lib/v1/types/utils/isa";
import type { SingleSeries } from "metabase-types/api";
import type { DatasetData, SingleSeries } from "metabase-types/api";
import type { SliceNameWidgetProps } from "./SliceNameWidget";
/**
* Weak on the dataset, which was the bulk of the old key, and a short string
* for the settings that actually change the result. The previous key was a
* JSON copy of every row and column, held for the life of the tab.
*/
const pieRowsByData = new WeakMap<
DatasetData,
Map<string, ReturnType<typeof getPieRows>>
>();
const pieRowsReadDeps = [
"pie.dimension",
"pie.metric",
@@ -101,24 +111,34 @@ export const PIE_CHART_DEFINITION: VisualizationDefinition = {
}),
"pie.rows": {
getHidden: () => true,
getValue: _.memoize(
(series, settings) => {
return getPieRows(series, settings, (value, options) =>
String(formatValue(value, options)),
);
},
([{ data }], settings) =>
JSON.stringify({
cols: data.cols,
rows: data.rows,
settings: _.pick(
settings,
...pieRowsReadDeps,
"pie.rows",
"pie.sort_rows_dimension",
),
}),
),
getValue: (series, settings) => {
const [{ data }] = series;
const settingsKey = JSON.stringify(
_.pick(
settings,
...pieRowsReadDeps,
"pie.rows",
"pie.sort_rows_dimension",
),
);
let rowsBySettings = pieRowsByData.get(data);
if (rowsBySettings == null) {
rowsBySettings = new Map();
pieRowsByData.set(data, rowsBySettings);
}
const cached = rowsBySettings.get(settingsKey);
if (cached !== undefined) {
return cached;
}
const pieRows = getPieRows(series, settings, (value, options) =>
String(formatValue(value, options)),
);
rowsBySettings.set(settingsKey, pieRows);
return pieRows;
},
readDependencies: pieRowsReadDeps,
writeDependencies: ["pie.sort_rows_dimension"],
},

61
frontend/src/metabase/visualizations/visualizations/PieChart/definition.unit.spec.ts Normal file
View File

@@ -0,0 +1,61 @@
import { PIE_CHART_DEFINITION } from "metabase/visualizations/visualizations/PieChart/definition";
import type { RawSeries, RowValues } from "metabase-types/api";
import {
createMockCard,
createMockColumn,
createMockDatasetData,
} from "metabase-types/api/mocks";
const ROWS = 200;
/**
* `pie.metric` and `pie.dimension` are absent, so getPieRows returns early with
* an empty array. That keeps these tests about the cache rather than about the
* row computation, which pie.unit.spec covers.
*/
const NO_SETTINGS = {};
function readPieRows(series: RawSeries): unknown {
const getValue = PIE_CHART_DEFINITION.settings?.["pie.rows"]?.getValue;
if (getValue == null) {
throw new Error(
"PIE_CHART_DEFINITION no longer exposes a pie.rows getValue",
);
}
return getValue(series, NO_SETTINGS);
}
function makeSeries(): RawSeries {
const rows: RowValues[] = Array.from({ length: ROWS }, (_, index) => [
`category ${index}`,
index,
]);
return [
{
card: createMockCard(),
data: createMockDatasetData({
rows,
cols: [
createMockColumn({ name: "category", display_name: "Category" }),
createMockColumn({ name: "count", display_name: "Count" }),
],
}),
},
];
}
describe("PieChart pie.rows caching", () => {
it("reuses the result for the same dataset and settings", () => {
const series = makeSeries();
expect(readPieRows(series)).toBe(readPieRows(series));
});
it("recomputes for a deep-equal but separate dataset", () => {
const series = makeSeries();
const rebuilt: RawSeries = JSON.parse(JSON.stringify(series));
expect(readPieRows(rebuilt)).not.toBe(readPieRows(series));
});
});

26
frontend/src/metabase/viz-core/lib/renderer_utils.leak.unit.spec.ts Normal file
View File

@@ -0,0 +1,26 @@
import { requireGarbageCollection, settleAndCollect } from "__support__/memory";
import { dayjs } from "metabase/dayjs";
import { parseXValue } from "./renderer_utils";
/** Parses one x value, then drops every reference to it. */
function makeParsedDayjsRef(): WeakRef<object> {
const parsed = parseXValue("2031-05-06", { isTimeseries: true });
if (!dayjs.isDayjs(parsed)) {
throw new Error("expected the timeseries path to return a Dayjs");
}
return new WeakRef(parsed);
}
describe("renderer_utils parsed x value caching", () => {
it("releases a parsed value once its caller is done", async () => {
requireGarbageCollection();
const parsed = makeParsedDayjsRef();
await settleAndCollect();
// The cache used to live at module scope, so one Dayjs per distinct
// timestamp stayed on the heap for the life of the tab.
expect(parsed.deref()).toBeUndefined();
});
});

80
frontend/src/metabase/viz-core/lib/renderer_utils.ts
View File

@@ -33,39 +33,66 @@ type ParseOptions = {
type Warn = (warning: VisualizationWarning) => void;
type ParsedXValue = {
parsedValue: RowValue | Dayjs;
warning?: VisualizationWarning;
};
/**
* Lives for one getXValues pass. Repeated values must come back as the
* identical object: getXValues dedupes through a Set, and a parsed timestamp is
* a Dayjs, which a Set compares by reference. A cache that evicts, or that is
* not shared across the series of one chart, silently produces duplicate x
* values.
*/
type ParsedXValueCache = Map<string, ParsedXValue>;
function parseXValueUncached(
xValue: RowValue,
{ isNumeric, isTimeseries, isQuantitative, unit }: ParseOptions,
): ParsedXValue {
// don't parse as timestamp if we're going to display as a quantitative
// scale, e.x. years and Unix timestamps
if (isTimeseries && !isQuantitative) {
return parseTimestampAndWarn(xValue, unit);
}
const parsedValue = isNumeric ? xValue : String(formatNullable(xValue));
return { parsedValue };
}
// we need typeof so "2" and 2 don't have the same cache key
function getParsedXValueKey(xValue: RowValue, options: ParseOptions): string {
return [xValue, typeof xValue, ...Object.values(options)].join();
}
function parseXValueWithCache(
xValue: RowValue,
options: ParseOptions,
cache: ParsedXValueCache,
): ParsedXValue {
const key = getParsedXValueKey(xValue, options);
const cached = cache.get(key);
if (cached !== undefined) {
return cached;
}
const parsed = parseXValueUncached(xValue, options);
cache.set(key, parsed);
return parsed;
}
export function parseXValue(
xValue: RowValue,
options: ParseOptions,
warn: Warn = () => undefined,
): RowValue | Dayjs {
const { parsedValue, warning } = memoizedParseXValue(xValue, options);
const { parsedValue, warning } = parseXValueUncached(xValue, options);
if (warning !== undefined) {
warn(warning);
}
return parsedValue;
}
const memoizedParseXValue = _.memoize(
(
xValue: RowValue,
{ isNumeric, isTimeseries, isQuantitative, unit }: ParseOptions,
): {
parsedValue: RowValue | Dayjs;
warning?: VisualizationWarning;
} => {
// don't parse as timestamp if we're going to display as a quantitative
// scale, e.x. years and Unix timestamps
if (isTimeseries && !isQuantitative) {
return parseTimestampAndWarn(xValue, unit);
}
const parsedValue = isNumeric ? xValue : String(formatNullable(xValue));
return { parsedValue };
},
// create cache key from args
// we need typeof so "2" and 2 don't have the same cache key
(x, options) => [x, typeof x, ...Object.values(options)].join(),
);
function getParseOptions({
data,
settings,
@@ -101,7 +128,8 @@ export function getXValues({
// if _raw isn't set then we already have the raw series
const rawSeries =
isObjectWithRaw(series) && series._raw ? series._raw : series;
const warn = () => undefined; // no op since warning in handled by getDatas
// warnings are handled by getDatas, so they are dropped here
const parsedXValues: ParsedXValueCache = new Map();
const uniqueValues = new Set<Exclude<RowValue, null | undefined> | Dayjs>();
let isAscending = true;
let isDescending = true;
@@ -120,7 +148,11 @@ export function getXValues({
if (canDisplayNull(settings) && row[columnIndex] === null) {
continue;
}
const value = parseXValue(row[columnIndex], parseOptions, warn);
const { parsedValue: value } = parseXValueWithCache(
row[columnIndex],
parseOptions,
parsedXValues,
);
if (lastValue !== undefined && value != null) {
isAscending = isAscending && (lastValue ?? 0) <= value;
isDescending = isDescending && value <= (lastValue ?? 0);

24
frontend/src/metabase/viz-core/lib/renderer_utils.unit.spec.ts
View File

@@ -226,9 +226,33 @@ describe("getXValues", () => {
.map((value) => value.format("YYYY-MM-DD"));
expect(formattedXValues).toEqual(["2019-01-02", "2019-01-03"]);
});
it("should dedupe a timestamp shared by two series of the same chart", () => {
// 4 rows, 3 distinct instants. Two independently parsed Dayjs values are
// distinct in a Set, so this only collapses to 3 while every series of the
// pass shares one cache.
expect(
getXValuesForRows(
[
[["2021-01-01"], ["2021-01-02"]],
[["2021-01-01"], ["2021-01-03"]],
],
{ "graph.x_axis.scale": "timeseries" },
),
).toHaveLength(3);
});
});
describe("parseXValue", () => {
it("should not intern parsed values across separate calls", () => {
const first = parseXValue("2024-03-01", { isTimeseries: true });
const second = parseXValue("2024-03-01", { isTimeseries: true });
expect(dayjs.isDayjs(first)).toBe(true);
expect(first).toEqual(second);
expect(first).not.toBe(second);
});
it("should use options as part of the cache key", () => {
const value1 = parseXValue("2018-08-23", { isTimeseries: true });
const value2 = parseXValue("2018-08-23", { isTimeseries: false });

5
frontend/src/metabase/viz-core/shared/components/RowChart/utils/data.ts
View File

@@ -1,9 +1,9 @@
import type { Series as D3Series } from "d3";
import * as d3 from "d3";
import { stack, stackOffsetDiverging, stackOffsetExpand } from "d3";
import _ from "underscore";
import { formatNullable } from "metabase/utils/formatting";
import { memoize } from "metabase/utils/memoize";
import type { SeriesInfo } from "../../../types/data";
import type { ContinuousScaleType } from "../../../types/scale";
@@ -94,11 +94,10 @@ export const calculateStackedBars = <TDatum>(
patchD3StackDataForLogScale(stackedSeries);
}
const getDatumExtent = _.memoize(
const getDatumExtent = memoize(
(stackedSeries: D3Series<TDatum, string>[], datumIndex: number) => {
return d3.extent(stackedSeries.flatMap((series) => series[datumIndex]));
},
(_series, datumIndex) => datumIndex,
);
const seriesData: SeriesData<TDatum, SeriesInfo>[] = multipleSeries.map(

36
frontend/test/__support__/memory.ts Normal file
View File

@@ -0,0 +1,36 @@
/**
* Helpers for proving that a cache releases what its callers are done with.
*
* Specs using these must run under `--expose-gc --runInBand`, otherwise there
* is no way to force a collection and a WeakRef never clears:
*
* bun run test-memory
*/
// Node attaches `gc` to the global object only under --expose-gc, so it is
// absent from the ambient typings.
const forceGc = (globalThis as { gc?: () => void }).gc;
/**
* Call this first. Without --expose-gc a retention test silently proves
* nothing, and a test that cannot measure must fail rather than report a
* vacuous pass.
*/
export function requireGarbageCollection() {
if (forceGc == null) {
throw new Error(
"This spec proves a value is collected and needs --expose-gc. Run `bun run test-memory`.",
);
}
}
/**
* Lets the frames still holding a just-returned value unwind, so a WeakRef to
* it can actually be collected. Without this the control arm of a retention
* test survives too and the test discriminates nothing.
*/
export async function settleAndCollect(): Promise<void> {
await new Promise((resolve) => setTimeout(resolve, 0));
forceGc?.();
forceGc?.();
}

133
jest.base.conf.js Normal file
View File

@@ -0,0 +1,133 @@
// @ts-check
// eslint-disable-next-line import/no-commonjs
const esmPackages = require("./jest.esm-packages.js");
const swcJestTransform = [
"@swc/jest",
{
jsc: {
// Jest runs on Node so we can target a modern engine and skip a bunch
// of polyfills/transforms that `env.targets: ["defaults"]` would emit.
target: "es2022",
loose: true,
parser: {
syntax: "typescript",
tsx: true,
},
transform: {
react: {
runtime: "automatic",
},
},
experimental: {
plugins: [
["@swc-contrib/mut-cjs-exports", {}],
["@swc/plugin-emotion", { sourceMap: false }],
],
},
},
module: {
type: "commonjs",
},
sourceMaps: "inline",
minify: false,
},
];
const baseConfig = {
transform: {
"^.+\\.m?[jt]sx?$": swcJestTransform,
},
moduleNameMapper: {
// Force jose to use Node.js runtime instead of browser runtime in jsdom environment.
// The browser runtime expects CryptoKey to be globally available, which jsdom doesn't provide.
"^jose$": "<rootDir>/node_modules/jose/dist/node/cjs/index.js",
// remend only declares an `import` export condition, so jest's CJS resolver can't find it.
"^remend$": "<rootDir>/node_modules/remend/dist/index.js",
"^build-configs/(.*)$": "<rootDir>/frontend/build/$1",
"\\.(css|less)$": "<rootDir>/frontend/test/__mocks__/styleMock.js",
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$":
"<rootDir>/frontend/test/__mocks__/fileMock.js",
"^cljs/(.*)$": "<rootDir>/target/cljs_dev/$1",
"\\.svg\\?(component|source)":
"<rootDir>/frontend/test/__mocks__/svgMock.tsx",
"csv-parse/browser/esm/sync":
"<rootDir>/node_modules/csv-parse/dist/cjs/sync",
"csv-stringify/browser/esm/sync":
"<rootDir>/node_modules/csv-stringify/dist/cjs/sync",
/**
* SDK components import root SDK folder (`embedding-sdk`) that contains the ee plugins.
* This isn't a problem in the core app because we seem to not import to entry file directly
* for any component under tests.
*/
"sdk-ee-plugins": "<rootDir>/frontend/src/metabase/plugins/noop.ts",
/**
* SDK iframe embedding imports the embedding sdk and its components.
* We want to exclude the SDK from the main app's bundle to reduce the bundle size.
*/
"sdk-iframe-embedding-ee-plugins":
"<rootDir>/frontend/src/metabase/utils/noop.ts",
"ee-plugins": "<rootDir>/frontend/src/metabase/utils/noop.ts",
"ee-overrides": "<rootDir>/frontend/src/metabase/utils/noop.ts",
/**
* Docs snippets are loaded as raw text (asset/source) in rspack.
* In Jest, mock them as plain strings.
*/
"^docs/embedding/sdk/snippets/(.*)$":
"<rootDir>/frontend/test/__mocks__/fileMock.js",
"docs/(.*)$": "<rootDir>/docs/$1",
},
transformIgnorePatterns: [
// Combined pattern for both flat and bun isolated node_modules structures
// - Flat: node_modules/<pkg>/ where <pkg> is NOT in esmPackages
// - Bun: node_modules/.bun/<pkg>@<ver>/ where <pkg> is NOT in esmPackages
`<rootDir>/node_modules/(?:\\.bun/(?!(${esmPackages.join("|")})@)|(?!\\.bun)(?!(${esmPackages.join("|")})/))`,
// CLJS files are already compiled CJS — skip transform entirely
"<rootDir>/target/cljs_dev/",
],
testPathIgnorePatterns: [
"<rootDir>/frontend/.*/.*.tz.unit.spec.{js,jsx,ts,tsx}",
"<rootDir>/release/.*",
],
testMatch: ["<rootDir>/**/*.unit.spec.{js,jsx,ts,tsx}"],
modulePaths: [
"<rootDir>/frontend/test",
"<rootDir>/frontend/src",
"<rootDir>/enterprise/frontend/src",
],
modulePathIgnorePatterns: [
"<rootDir>/target/cljs_release/.*",
"<rootDir>/target/classes/.*",
"<rootDir>/resources/frontend_client",
"<rootDir>/.*/__mocks__",
"<rootDir>/enterprise/frontend/src/custom-viz",
],
setupFiles: [
"<rootDir>/frontend/test/jest-setup.js",
"<rootDir>/frontend/test/metabase-bootstrap.js",
"<rootDir>/frontend/test/register-visualizations.js",
],
setupFilesAfterEnv: ["<rootDir>/frontend/test/jest-setup-env.js"],
globals: {
ga: {},
},
coverageDirectory: "./coverage",
collectCoverageFrom: [
"frontend/src/**/*.{js,jsx,ts,tsx}",
"enterprise/frontend/src/**/*.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.styled.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.story.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.info.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.unit.spec.{js,jsx,ts,tsx}",
],
coveragePathIgnorePatterns: [
"/node_modules/",
"/target/cljs_dev/",
"/target/cljs_release/",
"/frontend/test/",
],
testEnvironment: "jest-environment-jsdom",
};
// eslint-disable-next-line import/no-commonjs
module.exports = baseConfig;

138
jest.config.js
View File

@@ -1,133 +1,9 @@
// @ts-check
/** eslint-disable-next-line import/no-commonjs */
const esmPackages = require("./jest.esm-packages.js");
const swcJestTransform = [
"@swc/jest",
{
jsc: {
// Jest runs on Node so we can target a modern engine and skip a bunch
// of polyfills/transforms that `env.targets: ["defaults"]` would emit.
target: "es2022",
loose: true,
parser: {
syntax: "typescript",
tsx: true,
},
transform: {
react: {
runtime: "automatic",
},
},
experimental: {
plugins: [
["@swc-contrib/mut-cjs-exports", {}],
["@swc/plugin-emotion", { sourceMap: false }],
],
},
},
module: {
type: "commonjs",
},
sourceMaps: "inline",
minify: false,
},
];
const baseConfig = {
transform: {
"^.+\\.m?[jt]sx?$": swcJestTransform,
},
moduleNameMapper: {
// Force jose to use Node.js runtime instead of browser runtime in jsdom environment.
// The browser runtime expects CryptoKey to be globally available, which jsdom doesn't provide.
"^jose$": "<rootDir>/node_modules/jose/dist/node/cjs/index.js",
// remend only declares an `import` export condition, so jest's CJS resolver can't find it.
"^remend$": "<rootDir>/node_modules/remend/dist/index.js",
"^build-configs/(.*)$": "<rootDir>/frontend/build/$1",
"\\.(css|less)$": "<rootDir>/frontend/test/__mocks__/styleMock.js",
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$":
"<rootDir>/frontend/test/__mocks__/fileMock.js",
"^cljs/(.*)$": "<rootDir>/target/cljs_dev/$1",
"\\.svg\\?(component|source)":
"<rootDir>/frontend/test/__mocks__/svgMock.tsx",
"csv-parse/browser/esm/sync":
"<rootDir>/node_modules/csv-parse/dist/cjs/sync",
"csv-stringify/browser/esm/sync":
"<rootDir>/node_modules/csv-stringify/dist/cjs/sync",
/**
* SDK components import root SDK folder (`embedding-sdk`) that contains the ee plugins.
* This isn't a problem in the core app because we seem to not import to entry file directly
* for any component under tests.
*/
"sdk-ee-plugins": "<rootDir>/frontend/src/metabase/plugins/noop.ts",
/**
* SDK iframe embedding imports the embedding sdk and its components.
* We want to exclude the SDK from the main app's bundle to reduce the bundle size.
*/
"sdk-iframe-embedding-ee-plugins":
"<rootDir>/frontend/src/metabase/utils/noop.ts",
"ee-plugins": "<rootDir>/frontend/src/metabase/utils/noop.ts",
"ee-overrides": "<rootDir>/frontend/src/metabase/utils/noop.ts",
/**
* Docs snippets are loaded as raw text (asset/source) in rspack.
* In Jest, mock them as plain strings.
*/
"^docs/embedding/sdk/snippets/(.*)$":
"<rootDir>/frontend/test/__mocks__/fileMock.js",
"docs/(.*)$": "<rootDir>/docs/$1",
},
transformIgnorePatterns: [
// Combined pattern for both flat and bun isolated node_modules structures
// - Flat: node_modules/<pkg>/ where <pkg> is NOT in esmPackages
// - Bun: node_modules/.bun/<pkg>@<ver>/ where <pkg> is NOT in esmPackages
`<rootDir>/node_modules/(?:\\.bun/(?!(${esmPackages.join("|")})@)|(?!\\.bun)(?!(${esmPackages.join("|")})/))`,
// CLJS files are already compiled CJS — skip transform entirely
"<rootDir>/target/cljs_dev/",
],
testPathIgnorePatterns: [
"<rootDir>/frontend/.*/.*.tz.unit.spec.{js,jsx,ts,tsx}",
"<rootDir>/release/.*",
],
testMatch: ["<rootDir>/**/*.unit.spec.{js,jsx,ts,tsx}"],
modulePaths: [
"<rootDir>/frontend/test",
"<rootDir>/frontend/src",
"<rootDir>/enterprise/frontend/src",
],
modulePathIgnorePatterns: [
"<rootDir>/target/cljs_release/.*",
"<rootDir>/target/classes/.*",
"<rootDir>/resources/frontend_client",
"<rootDir>/.*/__mocks__",
"<rootDir>/enterprise/frontend/src/custom-viz",
],
setupFiles: [
"<rootDir>/frontend/test/jest-setup.js",
"<rootDir>/frontend/test/metabase-bootstrap.js",
"<rootDir>/frontend/test/register-visualizations.js",
],
setupFilesAfterEnv: ["<rootDir>/frontend/test/jest-setup-env.js"],
globals: {
ga: {},
},
coverageDirectory: "./coverage",
collectCoverageFrom: [
"frontend/src/**/*.{js,jsx,ts,tsx}",
"enterprise/frontend/src/**/*.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.styled.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.story.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.info.{js,jsx,ts,tsx}",
"!<rootDir>/**/*.unit.spec.{js,jsx,ts,tsx}",
],
coveragePathIgnorePatterns: [
"/node_modules/",
"/target/cljs_dev/",
"/target/cljs_release/",
"/frontend/test/",
],
testEnvironment: "jest-environment-jsdom",
};
const baseConfig = require("./jest.base.conf.js");
// Heap measurement specs. They run from jest.memory.conf.js, never from the
// default sharded run, because they need --expose-gc and a quiet heap.
const MEMORY_TEST_PATTERN = "\\.leak\\.unit\\.spec\\.";
/** @type {import('jest').Config} */
const config = {
@@ -152,6 +28,11 @@ const config = {
"<rootDir>/enterprise/frontend/src/embedding-sdk-ee/**/*.unit.spec.{ts,tsx}",
],
testPathIgnorePatterns: [
...(baseConfig.testPathIgnorePatterns || []),
MEMORY_TEST_PATTERN,
],
setupFiles: [
...baseConfig.setupFiles,
"<rootDir>/frontend/src/embedding-sdk-shared/jest/setup-env.ts",
@@ -178,6 +59,7 @@ const config = {
"<rootDir>/enterprise/frontend/src/embedding-sdk-ee",
"<rootDir>/enterprise/frontend/src/custom-viz",
"<rootDir>/frontend/lint/tests",
MEMORY_TEST_PATTERN,
],
},
{

24
jest.memory.conf.js Normal file
View File

@@ -0,0 +1,24 @@
// @ts-check
/**
* Heap retention specs.
*
* They need --expose-gc and a heap nobody else is touching, so they cannot join
* the sharded default run. Use `bun run test-memory`.
*/
// eslint-disable-next-line import/no-commonjs
const baseConfig = require("./jest.base.conf.js");
/** @type {import('jest').Config} */
// eslint-disable-next-line import/no-commonjs
module.exports = {
...baseConfig,
displayName: "memory",
setupFilesAfterEnv: [
...baseConfig.setupFilesAfterEnv,
"<rootDir>/frontend/test/jest-setup-env-core.js",
],
testMatch: ["<rootDir>/**/*.leak.unit.spec.{ts,tsx}"],
reporters: ["default"],
};

2
package.json
View File

@@ -130,7 +130,6 @@
"prosemirror-state": "^1.4.4",
"prosemirror-view": "^1.40.1",
"querystring-es3": "^0.2.1",
"re-reselect": "^4.0.1",
"react": "^18.2.0",
"react-ansi-style": "^1.0.0",
"react-color": "^2.14.1",
@@ -493,6 +492,7 @@
"test-cypress-host-sample-apps": "tsx ./e2e/runner/run_cypress_host_sample_apps.ts",
"test-debug": "bun run build:cljs && node --inspect-brk node_modules/.bin/jest --runInBand --detectOpenHandles",
"test-timezones": "bun install && ./frontend/test/__runner__/run_timezone_tests",
"test-memory": "node --expose-gc ./node_modules/.bin/jest --runInBand --config jest.memory.conf.js",
"test-timezones-unit": "jest --silent --maxWorkers=4 --config jest.tz.unit.conf.js",
"test-unit": "bun run build:cljs && jest --maxWorkers=4",
"test-unit-keep-cljs": "jest --maxWorkers=4",