From 4414bd960d7695b98b283c90f7233b3ce047401a Mon Sep 17 00:00:00 2001 From: Westbrook Johnson Date: Fri, 13 Oct 2023 19:08:36 -0400 Subject: [PATCH] fix(grid): plug a mememory leak from the render process --- tools/grid/src/Grid.ts | 7 ++- tools/grid/test/grid-memory.test.ts | 72 +++++++++++++++++++++++++++++ web-test-runner.config.js | 10 +++- web-test-runner.utils.js | 22 +++++++++ 4 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 tools/grid/test/grid-memory.test.ts diff --git a/tools/grid/src/Grid.ts b/tools/grid/src/Grid.ts index f208b06867..6cb3f9fa4c 100644 --- a/tools/grid/src/Grid.ts +++ b/tools/grid/src/Grid.ts @@ -17,6 +17,7 @@ import { PropertyValues, ReactiveElement, render, + RootPart, TemplateResult, } from '@spectrum-web-components/base'; import { property } from '@spectrum-web-components/base/src/decorators.js'; @@ -35,6 +36,8 @@ export class Grid extends LitVirtualizer { return [styles]; } + private __gridPart: RootPart | undefined = undefined; + @property({ type: String }) public focusableSelector!: string; @@ -147,13 +150,14 @@ export class Grid extends LitVirtualizer { } if (this.isConnected) { - render(super.render(), this); + this.__gridPart = render(super.render(), this); } super.update(changes); } override connectedCallback(): void { super.connectedCallback(); + this.__gridPart?.setConnected(true); this.addEventListener('change', this.handleChange, { capture: true }); } @@ -161,6 +165,7 @@ export class Grid extends LitVirtualizer { this.removeEventListener('change', this.handleChange, { capture: true, }); + this.__gridPart?.setConnected(false); super.disconnectedCallback(); } } diff --git a/tools/grid/test/grid-memory.test.ts b/tools/grid/test/grid-memory.test.ts new file mode 100644 index 0000000000..c703d3d2bc --- /dev/null +++ b/tools/grid/test/grid-memory.test.ts @@ -0,0 +1,72 @@ +/* +Copyright 2023 Adobe. All rights reserved. +This file is licensed to you under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. You may obtain a copy +of the License at http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under +the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +OF ANY KIND, either express or implied. See the License for the specific language +governing permissions and limitations under the License. +*/ + +import { expect, fixture, nextFrame } from '@open-wc/testing'; +import { html, render } from '@spectrum-web-components/base'; +import { Default } from '../stories/grid.stories'; + +async function usedHeapMB(): Promise { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const memorySample = performance.measureUserAgentSpecificMemory(); + return (await memorySample).bytes / (1024 * 1024); +} + +describe('Grid memory usage', () => { + it('releases references on disconnect', async function () { + if (!window.gc || !('measureUserAgentSpecificMemory' in performance)) + this.skip(); + + this.timeout(10000); + + const iterations = 50; + let active = false; + + const el = await fixture( + html` +
+ ` + ); + + async function toggle( + forced: boolean | undefined = undefined + ): Promise { + active = forced != null ? forced : !active; + render(active ? Default() : html``, el); + await nextFrame(); + await nextFrame(); + } + + // "shake things out" to get a good first reading + for (let i = 0; i < 5; i++) { + await toggle(); + } + await toggle(false); + const beforeMB = await usedHeapMB(); + + for (let i = 0; i < iterations; i++) { + await toggle(); + } + await toggle(false); + const afterMB = await usedHeapMB(); + + /** + * An actually leak here shapes up to be more than 10MB per test, + * we could be more linient later, if needed, but the test currently + * shows less heap after the test cycle. + */ + expect( + afterMB - beforeMB, + `before: ${beforeMB}, after: ${afterMB}` + ).to.be.lt(0); + }); +}); diff --git a/web-test-runner.config.js b/web-test-runner.config.js index 3d14c0ec62..e9b4d5a583 100644 --- a/web-test-runner.config.js +++ b/web-test-runner.config.js @@ -17,6 +17,7 @@ import { sendMousePlugin } from './test/plugins/send-mouse-plugin.js'; import { chromium, chromiumWithFlags, + chromiumWithMemoryTooling, configuredVisualRegressionPlugin, firefox, packages, @@ -54,6 +55,13 @@ export default { } }, }, + { + name: 'measureUserAgentSpecificMemory-plugin', + transform(context) { + context.set('Cross-Origin-Opener-Policy', 'same-origin'); + context.set('Cross-Origin-Embedder-Policy', 'credentialless'); + }, + }, ], mimeTypes: { '**/*.json': 'js', @@ -135,5 +143,5 @@ export default { }, ], group: 'unit', - browsers: [chromium, firefox, webkit], + browsers: [chromiumWithMemoryTooling, firefox, webkit], }; diff --git a/web-test-runner.utils.js b/web-test-runner.utils.js index 07a9fc4e60..097c921687 100644 --- a/web-test-runner.utils.js +++ b/web-test-runner.utils.js @@ -24,6 +24,28 @@ export const chromium = playwrightLauncher({ }), }); +export const chromiumWithMemoryTooling = playwrightLauncher({ + product: 'chromium', + createBrowserContext: ({ browser }) => + browser.newContext({ + ignoreHTTPSErrors: true, + permissions: ['clipboard-read', 'clipboard-write'], + }), + launchOptions: { + headless: false, + args: [ + '--js-flags=--expose-gc', + '--headless=new', + /** + * Cause `measureUserAgentSpecificMemory()` to GC immediately, + * instead of up to 20s later: + * https://web.dev/articles/monitor-total-page-memory-usage#local_testing + **/ + '--enable-blink-features=ForceEagerMeasureMemory', + ], + }, +}); + export const chromiumWithFlags = playwrightLauncher({ product: 'chromium', launchOptions: {