Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

improve rerender of vz_line_chart #3524

Merged
merged 1 commit into from
Apr 22, 2020
Merged

improve rerender of vz_line_chart #3524

merged 1 commit into from
Apr 22, 2020

Conversation

stephanwlee
Copy link
Contributor

We identified few several expensive operations going on underneath Plottable.

  • When we modified dataset() attached to a plot, Plottable recalculated
    domain which in turn triggered redraw of the plot.
  • Plot redraw is scheduled but repeated clearTimeout and setTimeout
    take considerable time which was the large part of the dataset update
  • Changing a dataset caused us to updateSpecialDataset N times (N =
    number of runs)

We addressed the issue by:

  • commit API after making all the changes
  • remember which runs/dataset were changed
  • update datasets only once per commit

Empirically, the render time:

  • toggling run on the selector went down from 1740ms to 314ms
  • triggering the log scale went down from 315ms to 264ms
    • from ~670ms to ~620ms when measured from mouse up

Do note that this change does not improve time for other charting
operation like zoom, smoothing changes, and etc...

Test plan: rendered the temperature demo and manually clicked around UI elements and compared the result against TensorBoard 2.1.1.

@stephanwlee
improve rerender of vz_line_chart
c91c30e 
We identified few several expensive operations going on underneath Plottable.
- When we modified dataset() attached to a plot, Plottable recalculated
  domain which in turn triggered redraw of the plot.
- Plot redraw is scheduled but repeated `clearTimeout` and `setTimeout`
  take considerable time which was the large part of the dataset update
- Changing a dataset caused us to updateSpecialDataset N times (N =
  number of runs)

We addressed the issue by:
- `commit` API after making all the changes
- remember which runs/dataset were changed
- update datasets only once per commit

We did not address:
- autoDomain recomputing the bounds on every data changes.
  - there is no programmatical way to disable auto domain unless we
    change the domain which causes, for instance,  zoom level to reset

Empirically, the render time:
- toggling run on the selector went down from 1740ms to 273ms
- triggering the log scale went down from 315ms to 264ms
  - from ~670ms to ~620ms when measured from mouse up

Do note that this change does not improve time for other charting
operation like zoom, smoothing changes, and etc...
@wchargin wchargin added the theme:performance Performance, scalability, large data sizes, slowness, etc. label Apr 17, 2020
@wchargin
Copy link
Contributor

Very nice. On the hparams demo’s batch_accuracy tag (50 time series),
I’m seeing a toggle-run improvement of 4.5 seconds → ~0.3 seconds. The
new version is fast enough that it’s hard for me to time it accurately
with my stopwatch.

@stephanwlee stephanwlee requested a review from bmd3k April 21, 2020 17:09
@stephanwlee stephanwlee merged commit 27d0023 into tensorflow:master Apr 22, 2020
@stephanwlee stephanwlee deleted the plottable branch April 22, 2020 15:39
@wchargin wchargin mentioned this pull request Apr 27, 2020
Closed
wchargin added a commit that referenced this pull request Apr 27, 2020
@wchargin
Revert "improve rerender of vz_line_chart (#3524)"
bfd9a23 
Summary:
This reverts commit 27d0023.

Fixes #3551.

Test Plan:
Opening the PR curves dashboard and sliding one of the step sliders now
correctly renders the PR curve instead of showing just the final datum.

wchargin-branch: revert-3524
wchargin added a commit that referenced this pull request Apr 27, 2020
@wchargin
Partially revert "improve rerender of vz_line_chart (#3524)" (#3552)
46b6e61 
Summary:
This reverts commit 27d0023, then
reinstates stubs for the new method definitions due to Google-internal
code that expects them to exist.

Stopgap for #3551 pending real fix.

Test Plan:
Opening the PR curves dashboard and sliding one of the step sliders now
correctly renders the PR curve instead of showing just the final datum.

wchargin-branch: revert-3524
@stephanwlee stephanwlee mentioned this pull request Apr 30, 2020
Merged
caisq pushed a commit to caisq/tensorboard that referenced this pull request May 19, 2020
@stephanwlee @caisq
improve rerender of vz_line_chart (tensorflow#3524)
22326ab 
We identified few several expensive operations going on underneath Plottable.
- When we modified dataset() attached to a plot, Plottable recalculated
  domain which in turn triggered redraw of the plot.
- Plot redraw is scheduled but repeated `clearTimeout` and `setTimeout`
  take considerable time which was the large part of the dataset update
- Changing a dataset caused us to updateSpecialDataset N times (N =
  number of runs)

We addressed the issue by:
- `commit` API after making all the changes
- remember which runs/dataset were changed
- update datasets only once per commit

We did not address:
- autoDomain recomputing the bounds on every data changes.
  - there is no programmatical way to disable auto domain unless we
    change the domain which causes, for instance,  zoom level to reset

Empirically, the render time:
- toggling run on the selector went down from 1740ms to 273ms
- triggering the log scale went down from 315ms to 264ms
  - from ~670ms to ~620ms when measured from mouse up

Do note that this change does not improve time for other charting
operation like zoom, smoothing changes, and etc...
caisq pushed a commit to caisq/tensorboard that referenced this pull request May 19, 2020
@wchargin @caisq
Partially revert "improve rerender of vz_line_chart (tensorflow#3524)" (
c33d4d2 
tensorflow#3552)

Summary:
This reverts commit 27d0023, then
reinstates stubs for the new method definitions due to Google-internal
code that expects them to exist.

Stopgap for tensorflow#3551 pending real fix.

Test Plan:
Opening the PR curves dashboard and sliding one of the step sliders now
correctly renders the PR curve instead of showing just the final datum.

wchargin-branch: revert-3524
caisq pushed a commit that referenced this pull request May 27, 2020
@stephanwlee @caisq
improve rerender of vz_line_chart (#3524)
73285f2 
We identified few several expensive operations going on underneath Plottable.
- When we modified dataset() attached to a plot, Plottable recalculated
  domain which in turn triggered redraw of the plot.
- Plot redraw is scheduled but repeated `clearTimeout` and `setTimeout`
  take considerable time which was the large part of the dataset update
- Changing a dataset caused us to updateSpecialDataset N times (N =
  number of runs)

We addressed the issue by:
- `commit` API after making all the changes
- remember which runs/dataset were changed
- update datasets only once per commit

We did not address:
- autoDomain recomputing the bounds on every data changes.
  - there is no programmatical way to disable auto domain unless we
    change the domain which causes, for instance,  zoom level to reset

Empirically, the render time:
- toggling run on the selector went down from 1740ms to 273ms
- triggering the log scale went down from 315ms to 264ms
  - from ~670ms to ~620ms when measured from mouse up

Do note that this change does not improve time for other charting
operation like zoom, smoothing changes, and etc...
caisq pushed a commit that referenced this pull request May 27, 2020
@wchargin @caisq
Partially revert "improve rerender of vz_line_chart (#3524)" (#3552)
6aeb239 
Summary:
This reverts commit 27d0023, then
reinstates stubs for the new method definitions due to Google-internal
code that expects them to exist.

Stopgap for #3551 pending real fix.

Test Plan:
Opening the PR curves dashboard and sliding one of the step sliders now
correctly renders the PR curve instead of showing just the final datum.

wchargin-branch: revert-3524
wchargin added a commit that referenced this pull request Aug 19, 2020
@wchargin
scalars: commit chart changes in batch
ef6d050 
Summary:
In #3524, we defined a batch API for chart updates to avoid useless
paints that would be immediately overwritten. We now take further
advantage of this API to only paint charts once all data has been
loaded.

Test Plan:
On the hparams demo directory, this brings paint time (post-network)
for four charts rendering 50 trials each down from about 8 seconds to
under 1 second.

wchargin-branch: scalars-batch-commit
wchargin-source: b0babe18b592f577803eebe6f4102c032fc9246f
@wchargin wchargin mentioned this pull request Aug 19, 2020
Merged
wchargin added a commit that referenced this pull request Aug 20, 2020
@wchargin
scalars: commit chart changes in batch (#4053)
19ce4f1 
Summary:
In #3524, we defined a batch API for chart updates to avoid useless
paints that would be immediately overwritten. We now take further
advantage of this API to only paint charts once all data has been
loaded.

Test Plan:
On the hparams demo directory, this brings paint time (post-network)
for four charts rendering 50 trials each down from about 8 seconds to
under 1 second.

wchargin-branch: scalars-batch-commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmd3k bmd3k bmd3k approved these changes

Assignees
No one assigned
Labels
cla: yes theme:performance Performance, scalability, large data sizes, slowness, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephanwlee @wchargin @bmd3k @googlebot