Eagerly lose WebGL context on `DrawWebGL.destroy()` #7080

davetsay · 2023-09-22T00:25:52Z

Describe your changes:

Eagerly lose WebGL contexts on destroy by using the WEBGL_lose_context extension, recommended as a WebGL best practice, in an attempt to free up WebGL contexts sooner after Plots are unmounted.

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-09-22T00:30:47Z

Current Playwright Test Results Summary

✅ 14 Passing - ⚠️ 1 Flaky

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

(Last updated on 10/03/2023 07:33:14pm UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: e2b7b83

Started: 10/03/2023 07:30:32pm UTC

⚠️ Flakes

📄 functional/plugins/displayLayout/displayLayout.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Display Layout When multiple plots are contained in a layout, we only ask for annotations once @couchdb Retry 1 • Initial Attempt	2.22% (1) 1 / 45 run failed over last 7 days	22.22% (10) 10 / 45 runs flaked over last 7 days

View Detailed Build Results

Current Playwright Test Results Summary

✅ 141 Passing - ⚠️ 1 Flaky

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

(Last updated on 10/03/2023 07:33:14pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: e2b7b83

Started: 10/03/2023 07:05:03pm UTC

⚠️ Flakes

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

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Snapshot image tests Can drop an image onto a notebook and create a new entry Retry 1 • Initial Attempt	16.84% (16) 16 / 95 runs failed over last 7 days	54.74% (52) 52 / 95 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-09-22T00:31:38Z

Codecov Report

Merging #7080 (e2b7b83) into master (d973140) will decrease coverage by 4.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7080      +/-   ##
==========================================
- Coverage   59.87%   55.26%   -4.61%     
==========================================
  Files         413      650     +237     
  Lines       12795    26100   +13305     
  Branches        0     2549    +2549     
==========================================
+ Hits         7661    14425    +6764     
- Misses       5134    10979    +5845     
- Partials        0      696     +696

Flag	Coverage Δ		*Carryforward flag
e2e-full	`41.91% <ø> (-0.01%)`	⬇️	Carriedforward from cad7688
e2e-stable	`57.24% <100.00%> (+0.02%)`	⬆️
unit	`48.73% <100.00%> (?)`

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

Files	Coverage Δ
src/plugins/plot/draw/DrawWebGL.js	`90.26% <100.00%> (+0.94%)`	⬆️

... and 383 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 d973140...e2b7b83. Read the comment docs.

- Recommended by Mozilla's [WebGL best practices]-(https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#lose_contexts_eagerly).

unlikelyzero · 2023-09-22T15:21:32Z

With this implementation, are you making the trade off that we will pay the penalty for creating the context on navigation to a complex display or map tool inspector?

If there are two complex plot displays with 16 plots each, in two separate chrome tabs, are we creating and destroying the WebGL contexts Every time a user uses a different tab?

akhenry · 2023-09-22T17:23:20Z

According to the documentation this Web GL extension will simulate a loss of context. Are we sure it's actually losing the context? Does it fix the issues we were seeing? - https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context

ozyx · 2023-09-26T18:40:07Z

According to the documentation this Web GL extension will simulate a loss of context. Are we sure it's actually losing the context? Does it fix the issues we were seeing? - https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context

Yes, this does fix the issue with losing WebGL context. It looks like it's marking the context for deletion and being picked up by GC much quicker. Although it "simulates" losing context (confusing wording?), it is recommended as a best practice.

ozyx · 2023-09-26T18:42:46Z

With this implementation, are you making the trade off that we will pay the penalty for creating the context on navigation to a complex display or map tool inspector?

We already pay this penalty on navigation. This change just marks the contexts for deletion so that the GC can pick it up quicker. There's no way to manually destroy a context completely in memory, it needs to be handled by the GC, according to WebGL docs (I don't have the link unfortunately).

If there are two complex plot displays with 16 plots each, in two separate chrome tabs, are we creating and destroying the WebGL contexts Every time a user uses a different tab?

No. I believe the contexts are actually per browser instance, so in this case there would be potentially up to 16 WebGL contexts from Open MCT, and 16 contexts that fall back to 2D.

jvigliotta

Looks good! Nice work!

scottbell

Had some difficulty checking out the branch due to numbers appearing as a commit hash to git, but was able to solve that using git checkout --track origin/7074.

Here's the behavior before, with the nasty warnings stating Too many active WebGL contexts.:

before.mov

Here's the behavior after, with WebGL contexts properly being trashed 🚮:

after.mov

ozyx and others added 5 commits September 20, 2023 16:09

🙅🚮␡openmct.components␡🚮🙅

299297c

Merge branch 'mct7074' into 7074

f31dd2e

test default for preserveDrawingBuffer

bfedd5d

revert test of preserveDrawingBuffer

ca7a081

fix: delete WebGL resources on destroy

34ed860

Merge branch 'master' into 7074

0d09e2b

ozyx marked this pull request as ready for review September 22, 2023 04:09

ozyx added this to the Target:3.0.2 milestone Sep 22, 2023

ozyx added 2 commits September 21, 2023 22:23

fix: eagerly lose WebGL contexts on destroy

45106df

- Recommended by Mozilla's [WebGL best practices]-(https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/WebGL_best_practices#lose_contexts_eagerly).

chore: lint:fix

cad7688

ozyx changed the title ~~Memory leak fixes for data visualization~~ Eagerly lose WebGL context on DrawWebGL.destroy() Sep 22, 2023

ozyx modified the milestones: Target:3.0.2, Target:3.1.0 Sep 22, 2023

ozyx added type:maintenance tests, chores, or project maintenance performance impacts or improves performance labels Sep 26, 2023

ozyx requested review from akhenry, shefalijoshi and scottbell September 29, 2023 18:03

jvigliotta self-requested a review October 3, 2023 19:02

jvigliotta approved these changes Oct 3, 2023

View reviewed changes

Merge branch 'master' into 7074

e2b7b83

jvigliotta enabled auto-merge (squash) October 3, 2023 19:03

scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 3, 2023

scottbell approved these changes Oct 3, 2023

View reviewed changes

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 3, 2023

jvigliotta merged commit d53d8d5 into master Oct 3, 2023

jvigliotta deleted the 7074 branch October 3, 2023 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eagerly lose WebGL context on `DrawWebGL.destroy()` #7080

Eagerly lose WebGL context on `DrawWebGL.destroy()` #7080

davetsay commented Sep 22, 2023 •

edited by scottbell

Loading

deploysentinel bot commented Sep 22, 2023 •

edited

Loading

Test Case Results

Test Case Results

codecov bot commented Sep 22, 2023 •

edited

Loading

unlikelyzero commented Sep 22, 2023

akhenry commented Sep 22, 2023

ozyx commented Sep 26, 2023

ozyx commented Sep 26, 2023 •

edited

Loading

jvigliotta left a comment

scottbell left a comment •

edited

Loading

Eagerly lose WebGL context on DrawWebGL.destroy() #7080

Eagerly lose WebGL context on DrawWebGL.destroy() #7080

Conversation

davetsay commented Sep 22, 2023 • edited by scottbell Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Sep 22, 2023 • edited Loading

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

unlikelyzero commented Sep 22, 2023

akhenry commented Sep 22, 2023

ozyx commented Sep 26, 2023

ozyx commented Sep 26, 2023 • edited Loading

jvigliotta left a comment

Choose a reason for hiding this comment

scottbell left a comment • edited Loading

Choose a reason for hiding this comment

Eagerly lose WebGL context on `DrawWebGL.destroy()` #7080

Eagerly lose WebGL context on `DrawWebGL.destroy()` #7080

davetsay commented Sep 22, 2023 •

edited by scottbell

Loading

deploysentinel bot commented Sep 22, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

ozyx commented Sep 26, 2023 •

edited

Loading

scottbell left a comment •

edited

Loading