DM-10830: Optimize the performance of multi-trace chart updates #397

loitly · 2017-06-23T18:22:03Z

https://jira.lsstcorp.org/browse/DM-10830
Explores ways to improve scatter chart performance, especially in Firefox.
To test:

build firefly in this branch
goto http://localhost:8080/firefly/demo/ffapi-highlevel-charttest.html
- edit line 155 of this file from 'radius: 300' -> 'radius: 1000' to get 4k rows

Issues to consider:

plot update instead of redraw did not increase performance enough, especially on larger plots.
scattergl support is not complete.
- latest update v1.28.2 released just a few days ago
- known bugs: gl2d known limitations plotly/plotly.js#130
  - affects us:
    - reversed axis not supported
    - yaxis right+log does not work all the time.
    - zoom select on color map with size very sluggish.
    - 'open' symbol types not working. ie. 'circle-open'
what should we do?

Below is a summary of measured time for a single row highlight:

     500 rows        GL                SVG
     browser    update    redraw    update    redraw
     chrome     <15       150       40-45     50
     firefox    17-22     150       170-200   210

     4k rows        GL                SVG
     browser    update    redraw    update    redraw
     chrome     15-20     180       215       230
     firefox    20-25     180       1200      1400

The code is committed with no scattergl and partial update enabled.
To test the various modes yourself, set sessionStorage variables in debugger console:
chartRedraw: set to always redraw full plot
scatterGL: set to convert scatter to scattergl

   sessionStorage.setItem('chartRedraw', 'true' | 'false')
   sessionStorage.setItem('scatterGL', 'true' | 'false')
   sessionStorage.clear()

tgoldina · 2017-06-26T16:25:29Z

when trying to add another firefly histogram to histogram chart, I get

TypeError: Cannot convert undefined or null to object
    at entries (<anonymous>)
    at doAdd (NewTracePanel.jsx:78)

tgoldina

The code looks good.

However, I would not switch to scattergl at the moment. It's too buggy. We might want to file the issue reports with Plotly for the things they do not have on their list.

In addition to the bugs mentioned before, I see some strange behavior with selected points. As soon as the selection is on, the highlight does not work. Also hover tips are gone at this point.

After I remove the selection, I can not change highlight from the chart. I can from the table, but I see two points highlighted.

Also, if I add symmetric errors, it looks like the upper error is missing and it's not positioned quite right.

With noScatterGL=true, the highlight is not correct either after the selection (if I try to highlight the selected point). It is fine when I add chartRedraw=false.

tgoldina · 2017-06-26T17:58:51Z

src/firefly/js/charts/ChartUtil.js

+            ['layout.yaxis.range'] : undefined, //clear out fixed range
+        };
+    }
+}


I noticed that the defaults behave differently for scatter and scattergl.
For example yaxis: {ticklen: 5 } shows the tick, but it overlaps with the tick label for scattergl. It looks like the middle of the tick label rather then the edge is positioned next to the tick. Looks like a Plotly bug. The workaround is not to show the axis line or ticks, and show the grid instead.

Also, in scattergl, the spikes (vertical and horizontal droplines meeting at the point) are shown by default, while they are not shown by default in scatter plot. For crowded charts these lines are convenient, but since the spikes do not extend all the way, the direction is wrong when axes are on the opposite sides. None of the spike attributes listed in the documentation take effect for scattergl at the moment. mirror axis attribute does not work either. So the workaround would be to remove the options for opposite axis location. (As well as for reverse.)

ejoliet · 2017-06-26T19:40:05Z

I didn't quite understand how to test with or without gl, let's talk about it so you can show me.

What i did is i open the Time series tool and play with it. Looks much more faster and responsive but still quite slow in Firefox with 3000 row of table. I think the partial update fix is a right approach but there must be plotly code that is not optimized. I think we need to have a meeting and discuss all about this issue of performance to plan what to do and have clear what we can fix and what we have to live with.

Also during my test, i discover a bug when the plot is a density plot, the time series plot get frozen and even uploading a new table doesn't make the plot goes away. I needed to refresh the browser. I'll test other tables.

tgoldina · 2017-06-26T19:59:20Z

src/firefly/js/charts/ui/PlotlyWrapper.jsx

+        const {lastInputTime} = layout;
+        if (lastInputTime && renderType ===  RenderType.NEW_PLOT && graphDiv.data) {
+            // omitting 'firefly' from data[*] for now
+            data = data.map((d) => omit(d, 'firefly'));


I think the mismatch in clicked and highlighted point when highlighting a selected point is caused by not passing firefly rowIdx array in plotly data. (For selected points I get row idx from plotly div, because we do not save them in data.) I think, we should omit firefly object for the purposes of calculating the difference, but it should not be dropped completely. We should still pass it with the partial or full update. Alternatively, we should store selected and highlighted traces in our store.

tgoldina · 2017-06-26T20:12:59Z

src/firefly/js/charts/ChartUtil.js

@@ -548,6 +548,19 @@ export function applyDefaults(chartData={}) {
    chartData.layout = merge(defaultLayout, chartData.layout);
 }

+const TRACE_COLORS = ['lightblue', 'green', 'purple', 'brown', 'yellow', 'red'];
+export function getNewTraceDefaults(type='', traceNum=0) {
+    if (type.includes(SCATTER)) {


I think, when the type is not SCATTER this function returns undefined, which is causing the type error, which I mentioned before (when we are trying to use the result as an array).

- defaults to partial update and no scattergl. - fix getNewTraceDefaults() returns undefined when type is not scatter

loitly · 2017-06-27T21:07:06Z

I've committed changes to this pull request. Please review again.

defaults to partial update and no scattergl.
fix getNewTraceDefaults() returns undefined when type is not scatter
from tgoldina:
fixes highlighting selected, bug due to shared data

- remove redundant restyle - fix highlighted point not showing up on trace switch. - use color-blind friendly colors

ejoliet · 2017-06-28T01:30:02Z

When i click around in timeseries tool result layout (from raw to pahse folded table back and forth), i see error in console:

 at lcManager 
 TypeError: Cannot read property '0' of undefined
    at http://localhost:8080/firefly/firefly-dev.js:65411:53
    at http://localhost:8080/firefly/firefly-dev.js:62641:29
    at Array.reduce (native)
    at http://localhost:8080/firefly/firefly-dev.js:62640:29
    at Array.findIndex (native)
    at findIndex (http://localhost:8080/firefly/firefly-dev.js:62639:67)
    at keepHighlightedRowSynced (http://localhost:8080/firefly/firefly-dev.js:195735:35)
    at handlePlotActive (http://localhost:8080/firefly/firefly-dev.js:194393:43)
    at lcManager$ (http://localhost:8080/firefly/firefly-dev.js:194217:38)
    at tryCatch (http://localhost:8080/firefly/firefly-dev.js:26266:41)

loitly · 2017-06-28T15:27:54Z

When i click around in timeseries tool result layout (from raw to pahse folded table back and forth), i see error in console:

timeseries is still using highchart. I don't think this pull request has an affect on it. Have you tried the same on 'dev' or 'rc' build to see if there's a regression bug there already?

ejoliet · 2017-06-29T01:25:08Z

Timeseries in firefly/dev is using plotly.

loitly · 2017-06-29T18:11:43Z

Timeseries in firefly/dev is using plotly.

Okay, I found the errors now. Although this is an existing bug which can be duplicated in ops and is not related to this PR, I went ahead and fixed it.

DM-10830: Optimize the performance of multi-trace chart updates

c8743ad

loitly requested review from ejoliet and tgoldina June 23, 2017 18:22

tgoldina requested changes Jun 26, 2017

View reviewed changes

tgoldina reviewed Jun 26, 2017

View reviewed changes

robyww assigned loitly Jun 27, 2017

tgoldina and others added 2 commits June 27, 2017 13:41

fixes highlighting selected, bug due to shared data

d76da65

DM-10830: fixes made per review

eadf0f8

- defaults to partial update and no scattergl. - fix getNewTraceDefaults() returns undefined when type is not scatter

DM-10830: additinal fixes

f1e8e66

- remove redundant restyle - fix highlighted point not showing up on trace switch. - use color-blind friendly colors

tgoldina approved these changes Jun 27, 2017

View reviewed changes

loitly added 2 commits June 27, 2017 16:54

- add back the non-supported symbols since we're not using GL.

f266326

- fix wrong use of boolean function.

ab8f390

- fixed FilterInfo.js fail when comparing sparsed array(rows).

464634f

ejoliet approved these changes Jun 29, 2017

View reviewed changes

loitly added 2 commits June 29, 2017 13:41

- remove old plotly.js file

313a4f5

Merge branch 'dev' into DM-10830_optimize_plotly_chart

99d2125

loitly merged commit ed4a756 into dev Jun 29, 2017

loitly deleted the DM-10830_optimize_plotly_chart branch June 29, 2017 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10830: Optimize the performance of multi-trace chart updates #397

DM-10830: Optimize the performance of multi-trace chart updates #397

loitly commented Jun 23, 2017 •

edited

Loading

tgoldina commented Jun 26, 2017

tgoldina left a comment •

edited

Loading

tgoldina Jun 26, 2017

ejoliet commented Jun 26, 2017

tgoldina Jun 26, 2017 •

edited

Loading

tgoldina Jun 26, 2017

loitly commented Jun 27, 2017

ejoliet commented Jun 28, 2017

loitly commented Jun 28, 2017 •

edited

Loading

ejoliet commented Jun 29, 2017

loitly commented Jun 29, 2017

DM-10830: Optimize the performance of multi-trace chart updates #397

DM-10830: Optimize the performance of multi-trace chart updates #397

Conversation

loitly commented Jun 23, 2017 • edited Loading

tgoldina commented Jun 26, 2017

tgoldina left a comment • edited Loading

Choose a reason for hiding this comment

tgoldina Jun 26, 2017

Choose a reason for hiding this comment

ejoliet commented Jun 26, 2017

tgoldina Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

tgoldina Jun 26, 2017

Choose a reason for hiding this comment

loitly commented Jun 27, 2017

ejoliet commented Jun 28, 2017

loitly commented Jun 28, 2017 • edited Loading

ejoliet commented Jun 29, 2017

loitly commented Jun 29, 2017

loitly commented Jun 23, 2017 •

edited

Loading

tgoldina left a comment •

edited

Loading

tgoldina Jun 26, 2017 •

edited

Loading

loitly commented Jun 28, 2017 •

edited

Loading