Disable auto reload if page is not visible #3483
Conversation
Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
bmd3k
changed the title
There was a problem hiding this comment.
FYI: there is a different implementation of autoReloadBehavior on the Angular side. https://github.com/tensorflow/tensorboard/blob/master/tensorboard/webapp/plugins/plugins_component.ts#L162-L172
| ); | ||
| }, | ||
| _doReload: function() { | ||
| if (this.reload == null) { |
There was a problem hiding this comment.
I didn't write this originally so can't say for sure: But I wouldn't be surprised if the check is purposefully "==" to also catch when this.reload is undefined.
| window.clearTimeout(this._autoReloadId); | ||
| document.removeEventListener( | ||
| 'visibilitychange', | ||
| this._handleVisibilityChange.bind(this) |
There was a problem hiding this comment.
I believe bind call creates a new reference and this is not removing the event listener attached at L55.
There was a problem hiding this comment.
Ya, yuck. I looked at how some other components handle this and picked what looks like the least-bad option.
There was a problem hiding this comment.
FWIW, would perhaps be good to update the
Reload data every 30s.settings panel text to reflect the fact that this is now more complex behavior, e.g. "Reload data every 30s while page is visible" or something?cc @GalOshri
I am following up with Gal on it. Will send a followup PR if he thinks the message should be updated. My personal opinion is that most people would understand the new behavior without the explanation .
| ); | ||
| }, | ||
| _doReload: function() { | ||
| if (this.reload == null) { |
There was a problem hiding this comment.
I didn't write this originally so can't say for sure: But I wouldn't be surprised if the check is purposefully "==" to also catch when this.reload is undefined.
| window.clearTimeout(this._autoReloadId); | ||
| document.removeEventListener( | ||
| 'visibilitychange', | ||
| this._handleVisibilityChange.bind(this) |
There was a problem hiding this comment.
Ya, yuck. I looked at how some other components handle this and picked what looks like the least-bad option.
I followed up with Gal. He doesn't think it's necessary to change the existing text. |
In #3483 I attempted to pause and restart auto reload behavior based on page visibility. Unfortunately a change I made to event handling just before merging into master inadvertently disabled the logic. The change happened in a seam in the code that was not covered by tests. This change fixes the event handling and improves the test coverage.
Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible. Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
In tensorflow#3483 I attempted to pause and restart auto reload behavior based on page visibility. Unfortunately a change I made to event handling just before merging into master inadvertently disabled the logic. The change happened in a seam in the code that was not covered by tests. This change fixes the event handling and improves the test coverage.
Note that this is the ng_tensorboard version of #3483. * Motivation for features / changes Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible. * Technical description of changes Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible. Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
In #3483 I attempted to pause and restart auto reload behavior based on page visibility. Unfortunately a change I made to event handling just before merging into master inadvertently disabled the logic. The change happened in a seam in the code that was not covered by tests. This change fixes the event handling and improves the test coverage.
…low#3498) Note that this is the ng_tensorboard version of tensorflow#3483. * Motivation for features / changes Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible. * Technical description of changes Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
Note that this is the ng_tensorboard version of #3483. * Motivation for features / changes Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible. * Technical description of changes Do not auto reload if document is not visible according to the Page Visibility API. When page again becomes visible perform an auto reload if one has been missed.
4 participants
Motivation for features / changes
Reduce the load on TensorBoard servers by disabling auto reload if the page is not considered visible.
Technical description of changes
Do not auto reload if document is not visible according to the Page
Visibility API. When page again becomes visible perform an auto reload
if one has been missed.
Detailed steps to verify changes work correctly (as executed by you)
Run a local TensorBoard.
Open localhost:6006.
Ensure option "Reload data every 30s." is selected.
Open detached network tab in chrome developer tools.
Watch for several minutes and observe that many network calls are made every 30s.
Open different tab in same window but with chrome developer tools still visible.
Watch for several minutes and observe that no network calls are made.
Switch back to TensorBoard tab and watch as network calls are immediately made and UI reloads graphs.
Continue to watch network tab as it again follows a 30s period for reload network calls.