line chart: recreate charts upon fatal renderer errors #5239

psybuzz · 2021-08-13T00:43:55Z

The line chart's internal ChartImpl notifies the line chart when it is a WebGL
renderer that loses its context due to the page having too many active WebGL
contexts at once (>30 for my device).

This change adds logic to treat those errors as fatal when the line chart needs
to be updated. Scalar cards now handle the fatal error by destroying and
recreating the entire line chart component, fixing the blank chart issue.

Manually checked that charts are not blank after scrolling through lots of line
charts (used the tagFilter to view 2x the line charts, since elements are
duplicated).

Alternatives considered: calling restoreContext() [1] on the context does not
consistently fix the blank charts. When it does, it appears to lose the original
context configuration (antialias: true, precision: 'highp', alpha: true), leading to
low quality visuals [2].

Googlers, see b/196294346.

[1] https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/restoreContext
[2] https://imgur.com/a/kKnhqYJ

Diffbase: #5237
Followup: none

stephanwlee · 2021-08-13T04:44:25Z

Would you have a material you can share on whether this is happening because of too many contexts? Casual Google search tells me that this can happen when a canvas becomes invisible while first creating/drawing it. Also there was some conversation about the library attempting to recover from the event automatically mrdoob/three.js#5857 (comment). Even with the remediation, it is believable that Three.js will fail to recover when the canvas leaves the viewport but, again, I do not get an impression that this happens because of # of contexts (unless your memory is running out and browser is eagerly removing it).

If this truly happens only when becoming invisible while drawing the first frame, I think we can do away without propagating the information all the way up to scalar_card_component but instead have internal state in line_chart_component.ts and based on the disableUpdate value, we can decide whether to reattempt to bootstrap or not. WDYT?

Each line chart component in the Time Series dashboard gets its own canvas and webgl rendering context. After scrolling through too many line charts (for my device after ~30), the page issues warnings in the console like "Too many active WebGL contexts. Oldest context will be lost". When the browser decides there are too many contexts, it disposes old ones causing blank line charts. This change adds an event dispatched by the line chart's internal `ChartImpl` to notify clients when the rendering context is lost. This only applies to the WebGL renderer for now. A followup change addresses the remediation of blank scalar cards. Googlers, see b/196294346. Followup: #5239

psybuzz · 2021-08-16T22:17:11Z

From what I've seen manually, blank charts can be consistently triggered well after the canvas is initially drawn and visible. In the attached screencast, TensorBoard loads the dashboard containing 100s of scalar cards, then after scrolling through about 30-40, we scroll 3 scalar cards into view, and observe 3 "too many contexts" warnings in the console. After scrolling up, we can hide and reshow all the original cards (via entering a filter and removing the tag filter). Doing so produces exactly 3 blank cards, suggesting that for all these cards that were originally drawn with a visible DOM element, the only difference between a blank one and a non-blank one after the repro is the blank ones were the ones 'evicted' by the browser.

Based on that, I think it's safe to say that listening to some 'context lost' signal is necessary. It's unclear to me what the exact max limit on webgl contexts is or whether it's dependent on GPU memory, but this crbug suggests it might be device dependent in the future.

webglContextLossBlankChart.webm.zip

After scrolling through more and more cards, the warning just keep piling on.

I think we can do away without propagating the information all the way up to scalar_card_component but instead have internal state in line_chart_component.ts and based on the disableUpdate value, we can decide whether to reattempt to bootstrap or not. WDYT?

It might be possible to re-bootstrap the line chart from within line_chart_component without making scalar_card_component aware of things. However, given how stateful the LineChartComponent is, I opted to do the 'full re-creation' in ScalarCardComponent, as it is much less complexity.

stephanwlee

At the very least, can we make it so that the context loss issue is contained in the LineChartComponent (and it "blinks" using its own ngIf) instead of propagating it all the way up to the ScalarCardComponent?

Also, if your empirical and crbugs are correct, then depending on screen, this bug will happen no matter what if there are so many charts above the fold, right? Perhaps we should be using half SVG and half Canvas in the future.

psybuzz · 2021-08-17T22:49:31Z

Ready for another look!

At the very least, can we make it so that the context loss issue is contained in the LineChartComponent (and it "blinks" using its own ngIf) instead of propagating it all the way up to the ScalarCardComponent?

Done.

Also, if your empirical and crbugs are correct, then depending on screen, this bug will happen no matter what if there are so many charts above the fold, right?

Yes. After hiding and showing cards (e.g. enter the filtered view then clear the filter), the early cards were blank.

Perhaps we should be using half SVG and half Canvas in the future.

Yeah, something to consider in the future. This webgl limitation was quite surprising to me, and I think TensorBoard's use case is fairly unique among webapps.

stephanwlee · 2021-08-18T00:34:44Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component.ts

+  private onContextLost() {
+    // Since context may be lost when the component is hidden or does not need
+    // updates, the teardown and recreation happens lazily.
+    this.isRenderingContextLost = true;


For some reason, I thought you'd dispose lineChart here and set it to this.lineChart = null. Would that make the code more readable in anyway?

Done, yes a bit more expected.

stephanwlee · 2021-08-18T00:36:25Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component.ts

  /**
-   * Minimally and imperatively updates the chart library depending on prop changed.
+   * Minimally and imperatively updates the chart library depending on prop
+   * changed. When adding new `lineChart.*()` calls, keep this in sync with


I do not see any lineChart.* API invoked in recoverRendererIfNeeded. Do you truly need them to be synced?

Updated the comment to be this.lineChart.set*(). If we added a this.lineChart.setZMode(...) inside updateLineChart one day, we'd need the corresponding flag this.zModeUpdated = true inside recoverRendererIfNeeded so that any changes to initial values of this hypothetical "zMode" would persist after recovery.

stephanwlee · 2021-08-18T00:49:35Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component_test.ts

+      expect(fixture.componentInstance.chart.getLineChartForTest()).toBe(
+        chartBefore
+      );
+      expect(disposeSpy).toHaveBeenCalledTimes(0);
+      expectChartUpdateSpiesToHaveBeenCalledTimes(1);


Unclear whether these assertions are truly testing whether the line chart is attempting to recover. I guess assertion of lineChartBefore === lineChartAfter is the one? If so, put a semantic name on the assertion method so we can change the assertion easily.

Good idea, done.

psybuzz

Thanks for the review!

psybuzz · 2021-08-18T01:46:00Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component.ts

+  private onContextLost() {
+    // Since context may be lost when the component is hidden or does not need
+    // updates, the teardown and recreation happens lazily.
+    this.isRenderingContextLost = true;


Done, yes a bit more expected.

psybuzz · 2021-08-18T01:48:07Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component.ts

  /**
-   * Minimally and imperatively updates the chart library depending on prop changed.
+   * Minimally and imperatively updates the chart library depending on prop
+   * changed. When adding new `lineChart.*()` calls, keep this in sync with


Updated the comment to be this.lineChart.set*(). If we added a this.lineChart.setZMode(...) inside updateLineChart one day, we'd need the corresponding flag this.zModeUpdated = true inside recoverRendererIfNeeded so that any changes to initial values of this hypothetical "zMode" would persist after recovery.

psybuzz · 2021-08-18T01:48:22Z

tensorboard/webapp/widgets/line_chart_v2/line_chart_component_test.ts

+      expect(fixture.componentInstance.chart.getLineChartForTest()).toBe(
+        chartBefore
+      );
+      expect(disposeSpy).toHaveBeenCalledTimes(0);
+      expectChartUpdateSpiesToHaveBeenCalledTimes(1);


Good idea, done.

Each line chart component in the Time Series dashboard gets its own canvas and webgl rendering context. After scrolling through too many line charts (for my device after ~30), the page issues warnings in the console like "Too many active WebGL contexts. Oldest context will be lost". When the browser decides there are too many contexts, it disposes old ones causing blank line charts. This change adds an event dispatched by the line chart's internal `ChartImpl` to notify clients when the rendering context is lost. This only applies to the WebGL renderer for now. A followup change addresses the remediation of blank scalar cards. Googlers, see b/196294346. Followup: tensorflow#5239

The line chart's internal `ChartImpl` notifies the line chart when it is a WebGL renderer that loses its context due to the page having too many active WebGL contexts at once (>30 for my device). This change adds logic to treat those errors as fatal when the line chart needs to be updated. Scalar cards now handle the fatal error by destroying and recreating the entire line chart component, fixing the blank chart issue. Manually checked that charts are not blank after scrolling through lots of line charts (used the tagFilter to view 2x the line charts, since elements are duplicated). Alternatives considered: calling restoreContext() [1] on the context does not consistently fix the blank charts. When it does, it appears to lose the original context configuration (antialias: true, precision: 'highp', alpha: true), leading to low quality visuals [2]. Googlers, see b/196294346. [1] https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/restoreContext [2] https://imgur.com/a/kKnhqYJ Diffbase: tensorflow#5237

Each line chart component in the Time Series dashboard gets its own canvas and webgl rendering context. After scrolling through too many line charts (for my device after ~30), the page issues warnings in the console like "Too many active WebGL contexts. Oldest context will be lost". When the browser decides there are too many contexts, it disposes old ones causing blank line charts. This change adds an event dispatched by the line chart's internal `ChartImpl` to notify clients when the rendering context is lost. This only applies to the WebGL renderer for now. A followup change addresses the remediation of blank scalar cards. Googlers, see b/196294346. Followup: tensorflow#5239

The line chart's internal `ChartImpl` notifies the line chart when it is a WebGL renderer that loses its context due to the page having too many active WebGL contexts at once (>30 for my device). This change adds logic to treat those errors as fatal when the line chart needs to be updated. Scalar cards now handle the fatal error by destroying and recreating the entire line chart component, fixing the blank chart issue. Manually checked that charts are not blank after scrolling through lots of line charts (used the tagFilter to view 2x the line charts, since elements are duplicated). Alternatives considered: calling restoreContext() [1] on the context does not consistently fix the blank charts. When it does, it appears to lose the original context configuration (antialias: true, precision: 'highp', alpha: true), leading to low quality visuals [2]. Googlers, see b/196294346. [1] https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/restoreContext [2] https://imgur.com/a/kKnhqYJ Diffbase: tensorflow#5237

google-cla bot added the cla: yes label Aug 13, 2021

psybuzz mentioned this pull request Aug 13, 2021

line chart: introduce onContextLost renderer callback #5237

Merged

psybuzz marked this pull request as ready for review August 13, 2021 01:21

psybuzz mentioned this pull request Aug 13, 2021

timeseries: persist tag filter in the URL #5236

Merged

line chart: recreate charts upon fatal errors

a2b122d

psybuzz force-pushed the ts-webgl-restore2 branch from cf07b0b to a2b122d Compare August 16, 2021 21:05

psybuzz requested a review from stephanwlee August 16, 2021 22:17

stephanwlee reviewed Aug 17, 2021

View reviewed changes

psybuzz force-pushed the ts-webgl-restore2 branch from 80613b4 to f0dfe0b Compare August 17, 2021 22:38

ac: no scalar card

2c3794d

psybuzz force-pushed the ts-webgl-restore2 branch from f0dfe0b to 2c3794d Compare August 17, 2021 22:41

psybuzz requested a review from stephanwlee August 17, 2021 22:49

stephanwlee approved these changes Aug 18, 2021

View reviewed changes

ac2

3b62193

psybuzz commented Aug 18, 2021

View reviewed changes

psybuzz merged commit 82cfd86 into tensorflow:master Aug 18, 2021

line chart: recreate charts upon fatal renderer errors #5239

line chart: recreate charts upon fatal renderer errors #5239

Uh oh!

Conversation

psybuzz commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephanwlee commented Aug 13, 2021

Uh oh!

psybuzz commented Aug 16, 2021

Uh oh!

stephanwlee left a comment

Choose a reason for hiding this comment

Uh oh!

psybuzz commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psybuzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

psybuzz commented Aug 13, 2021 •

edited

Loading