Misc memory leak fixes #7224

ozyx · 2023-11-09T23:22:57Z

N/A

Describe your changes:

Various memory leak and performance related fixes discovered while investigating complex displays memory/cpu usage

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

deploysentinel · 2023-11-09T23:27:09Z

Current Playwright Test Results Summary

✅ 153 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/13/2023 09:09:44pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 287d929

Started: 11/13/2023 09:02:24pm UTC

⚠️ Flakes

📄 functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit Retry 1 • Initial Attempt	0% (0) 0 / 130 runs failed over last 7 days	48.46% (63) 63 / 130 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-11-13T19:27:13Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a9ad0bf) 55.31% compared to head (bc94f17) 55.41%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7224      +/-   ##
==========================================
+ Coverage   55.31%   55.41%   +0.09%     
==========================================
  Files         671      671              
  Lines       27002    27010       +8     
  Branches     2630     2631       +1     
==========================================
+ Hits        14936    14967      +31     
+ Misses      11344    11323      -21     
+ Partials      722      720       -2

Flag	Coverage Δ		*Carryforward flag
e2e-full	`41.15% <ø> (-0.26%)`	⬇️	Carriedforward from 47c3f0b
e2e-stable	`59.12% <90.00%> (+0.01%)`	⬆️
unit	`48.56% <23.07%> (+0.16%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
src/plugins/LADTable/LADTableView.js	`93.75% <100.00%> (+0.41%)`	⬆️
src/plugins/condition/ConditionSetViewProvider.js	`95.65% <100.00%> (+0.19%)`	⬆️
src/plugins/displayLayout/plugin.js	`91.66% <100.00%> (ø)`
src/plugins/plot/MctPlot.vue	`35.98% <100.00%> (ø)`
src/plugins/plot/MctTicks.vue	`75.00% <ø> (+0.30%)`	⬆️
src/plugins/plot/axis/XAxis.vue	`53.84% <ø> (-1.16%)`	⬇️
src/plugins/plot/axis/YAxis.vue	`73.41% <ø> (ø)`
src/plugins/plot/inspector/PlotOptionsBrowse.vue	`83.87% <ø> (ø)`
src/plugins/plot/inspector/PlotOptionsEdit.vue	`53.12% <ø> (ø)`
src/plugins/plot/inspector/PlotOptionsItem.vue	`75.00% <ø> (ø)`
... and 5 more

... and 11 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9ad0bf...bc94f17. Read the comment docs.

scottbell · 2024-02-08T08:25:41Z

src/plugins/plot/inspector/PlotOptionsBrowse.vue

@@ -23,7 +23,7 @@
  <div v-if="loaded" class="js-plot-options-browse">
    <ul v-if="!isStackedPlotObject" class="c-tree" aria-label="Plot Series Properties">
      <h2 class="--first" title="Plot series display properties in this object">Plot Series</h2>
-      <plot-options-item v-for="series in plotSeries" :key="series.key" :series="series" />
+      <PlotOptionsItem v-for="series in plotSeries" :key="series.keyString" :series="series" />


oof, nice catch

scottbell

Works well. Tested various components for functionality, and changes make sense from a memory perspective.

scottbell · 2024-02-08T08:42:24Z

src/plugins/viewLargeAction/viewLargeAction.js

@@ -99,7 +99,10 @@ export default class ViewLargeAction {
      }
    );
    this.preview = vNode.componentInstance;
-    this.destroy = destroy;
+    this.destroy = () => {


Not really applicable to this PR, but it looks like PreviewContainer throws a Vue warning when reusing an existing component (e.g., using viewLargeAction in a DisplayLayout):

[Vue warn]: Maximum recursive updates exceeded. This means you have a reactive effect that is mutating its own dependencies and thus recursively triggering itself. Possible sources include component template, render function, updated hook or watcher source function. ... repeating a zillion times ... flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 flushJobs @ runtime-core.esm-bundler.js:372 Promise.then (async) queueFlush @ runtime-core.esm-bundler.js:275 queuePostFlushCb @ runtime-core.esm-bundler.js:295 queueEffectWithSuspense @ runtime-core.esm-bundler.js:1665 componentUpdateFn @ runtime-core.esm-bundler.js:5770 run @ reactivity.esm-bundler.js:178 instance.update @ runtime-core.esm-bundler.js:5861 setupRenderEffect @ runtime-core.esm-bundler.js:5869 mountComponent @ runtime-core.esm-bundler.js:5659 processComponent @ runtime-core.esm-bundler.js:5612 patch @ runtime-core.esm-bundler.js:5087 mountChildren @ runtime-core.esm-bundler.js:5331 mountElement @ runtime-core.esm-bundler.js:5238 processElement @ runtime-core.esm-bundler.js:5203 patch @ runtime-core.esm-bundler.js:5075 mountChildren @ runtime-core.esm-bundler.js:5331 mountElement @ runtime-core.esm-bundler.js:5238 processElement @ runtime-core.esm-bundler.js:5203 patch @ runtime-core.esm-bundler.js:5075 componentUpdateFn @ runtime-core.esm-bundler.js:5755 run @ reactivity.esm-bundler.js:178 instance.update @ runtime-core.esm-bundler.js:5861 setupRenderEffect @ runtime-core.esm-bundler.js:5869 mountComponent @ runtime-core.esm-bundler.js:5659 processComponent @ runtime-core.esm-bundler.js:5612 patch @ runtime-core.esm-bundler.js:5087 mountChildren @ runtime-core.esm-bundler.js:5331 mountElement @ runtime-core.esm-bundler.js:5238 processElement @ runtime-core.esm-bundler.js:5203 patch @ runtime-core.esm-bundler.js:5075 componentUpdateFn @ runtime-core.esm-bundler.js:5755 run @ reactivity.esm-bundler.js:178 instance.update @ runtime-core.esm-bundler.js:5861 setupRenderEffect @ runtime-core.esm-bundler.js:5869 mountComponent @ runtime-core.esm-bundler.js:5659 processComponent @ runtime-core.esm-bundler.js:5612 patch @ runtime-core.esm-bundler.js:5087 componentUpdateFn @ runtime-core.esm-bundler.js:5755 run @ reactivity.esm-bundler.js:178 instance.update @ runtime-core.esm-bundler.js:5861 setupRenderEffect @ runtime-core.esm-bundler.js:5869 mountComponent @ runtime-core.esm-bundler.js:5659 processComponent @ runtime-core.esm-bundler.js:5612 patch @ runtime-core.esm-bundler.js:5087 render @ runtime-core.esm-bundler.js:6379 mount @ runtime-core.esm-bundler.js:3832 app.mount @ runtime-dom.esm-bundler.js:1449 mount @ mount.js:10 _getPreview @ viewLargeAction.js:81 _expand @ viewLargeAction.js:65 invoke @ viewLargeAction.js:51 action.onItemClicked @ MenuAPI.js:81 callWithErrorHandling @ runtime-core.esm-bundler.js:158 callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166 invoker @ runtime-dom.esm-bundler.js:595 eventHelpers.js:38 [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event. Consider marking event handler as 'passive' to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952 listenTo @ eventHelpers.js:38 initCanvas @ MctPlot.vue:885 callWithErrorHandling @ runtime-core.esm-bundler.js:158 callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166 emit @ runtime-core.esm-bundler.js:669 visibilityChanged @ MctChart.vue:300

Seems to happen with any type of component in a DisplayLayout, so probably an issue with PreviewContainer.

ozyx changed the title ~~Memlab 2021~~ Misc memory leak fixes Dec 11, 2023

unlikelyzero added the type:maintenance tests, chores, or project maintenance label Dec 14, 2023

ozyx force-pushed the memlab-2021 branch from 287d929 to 76a63a3 Compare February 6, 2024 21:01

ozyx added 6 commits February 6, 2024 13:06

fix(leak): remove font listeners

98744f5

fix(leak): release componentInstance

d701879

fix(perf): remove unused emit

1057453

fix(perf): only emit if tickWidth changes

80e7998

fix: warnings and undefined keys

18c0f54

fix: remove unused bind

3bc7993

ozyx force-pushed the memlab-2021 branch from 76a63a3 to 3bc7993 Compare February 6, 2024 21:06

ozyx added this to the Target:4.0.0 milestone Feb 6, 2024

ozyx requested review from scottbell and akhenry February 6, 2024 21:09

ozyx marked this pull request as ready for review February 6, 2024 21:09

ozyx and others added 2 commits February 6, 2024 13:19

fix: restore MctTicks component

66d5a8e

Merge branch 'master' into memlab-2021

47c3f0b

scottbell reviewed Feb 8, 2024

View reviewed changes

scottbell approved these changes Feb 8, 2024

View reviewed changes

Merge branch 'master' into memlab-2021

bc94f17

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Feb 14, 2024

ozyx enabled auto-merge (squash) February 14, 2024 23:47

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Feb 14, 2024

ozyx merged commit e086332 into master Feb 14, 2024
19 of 21 checks passed

ozyx deleted the memlab-2021 branch February 14, 2024 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc memory leak fixes #7224

Misc memory leak fixes #7224

ozyx commented Nov 9, 2023 •

edited

Loading

deploysentinel bot commented Nov 9, 2023 •

edited

Loading

Test Case Results

codecov bot commented Nov 13, 2023 •

edited

Loading

scottbell Feb 8, 2024

scottbell left a comment

scottbell Feb 8, 2024

scottbell Feb 8, 2024

Misc memory leak fixes #7224

Misc memory leak fixes #7224

Conversation

ozyx commented Nov 9, 2023 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Nov 9, 2023 • edited Loading

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

scottbell Feb 8, 2024

Choose a reason for hiding this comment

scottbell left a comment

Choose a reason for hiding this comment

scottbell Feb 8, 2024

Choose a reason for hiding this comment

scottbell Feb 8, 2024

Choose a reason for hiding this comment

ozyx commented Nov 9, 2023 •

edited

Loading

deploysentinel bot commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading