The performance of the chart editor is subpar when using a less performant device (like a laptop) and typing fast in a text field of the OptionsPanelContainer.
It is best observed in the preconfigured Stock-chart, as it has a lot of data. To simulate (very fast) typing, delete the prefilled "Lorem ipsum dolor sit amet" in the title by clicking at the end of the text input and deleting the value letter by letter by keeping the Backspace key pressed.
While it seems performant enough for most use cases, I believe this to be the bottleneck for comfortably working with larger data and more computationally expensive charts on less powerful machines.
The chart editor of everviz.com
Take this analysis with a big grain of salt. I noticed a significant performance drop when testing with the devTools open vs. closed. Also, using the chrome browser shipped with vscode resulted in worse performance - probably due to some development options enabled.
All tests have been made with the default Line-chart. I expect the performance to worsen with the Stock-chart, as it has much more data. A small test showed that the initialization of the chart took roughly double as long.
OS: Windows 11 Pro 10.0.22621
CPU: 16 × AMD Ryzen 7 PRO 4750U with Radeon Graphics
RAM: 31976MB
Chrome: 110.0.5481.177 (Official Build) (64-bit)
I analyzed deleting the title via a pressed Backspace key with the chrome performance tools.
Screenshot of the performance diagram of the 1 + 25 continuous Backspace presses
A typical stack trace of a single key press
For each key press, multiple tasks are executed:
| Task | Time |
|---|---|
sentry |
ca. 6ms |
Redux & Redux saga |
ca. 4ms |
Highcharts |
ca. 67ms |
ReactDOM |
ca. 35ms |
sentry |
ca. 17ms |
| TOTAL: | ca. 135ms |
Note that for each key press, sentry logged some errors. This could potentially explain the performance difference between having the development tools open or not. They could have already been fixed in sentry-javascript@7.35.0, which was released a month ago.
When observing the DOM, one can also see that the highcharts-container element is recreated on every key press. A closer look at the Highcharts task shows that the previous chart instance is first destroyed before the updated chart is rendered. This takes ca. 6ms. Note that the big parseCSV function itself is very short (0.2ms), but everything else is happening in the afterComplete hook that is invoked by the parseCSV.
A closer look at the Highcharts task
To further analyze the problem, I downloaded the sourcemaps automatically generated by webpack.
A lot of the frontend code can be seen in the sourcemaps
Note: If it is unintended behavior that everyone can see an unminified version of the frontend code, including comments and typescript files, you can disable it (look here and here).
The call stack led me to the initChart-saga in src\pages\ChartEditorPage\middleware\ChartEditor.js. It showed that the chart is reinitialized on every key press. It seemed to me that all chart modifications are done via this saga.
The performance problem is most noticeable when typing fast. Therefore, the frequency of chart reinitialization could be reduced by only reinitializing it when the user finished typing. In addition, this could also reduce the overhead of sentry, as the number of DOM manipulations and (depending on the implementation) the number of Redux-actions would be reduced.
The onChange event-emitter in the RichtextGroupWidget in src\shared\widgets\richtextgroup\RichtextGroupWidget.tsx could be debounced. To do this, one would probably have to introduce a component internal value variable holding the actual value of the input that can be different from the option in the chart. The reemit of onChange could then be debounced, e.g., via lodash.debounce.
This would also impact an undo/redo functionality, as instead of undoing inputs letter by letter, it is undone in short groups, depending on the typing speed and typing fluency of the user.
I implemented something similar for many inputs in an Angular project and found the resulting short desynchronization of the input value and the rest of the application to be acceptable. This can also be combined with only emitting valid values (e.g., instead of emitting a value that doesn't match some regex and potentially making the settings state inconsistent, the value is only emitted after it has been corrected by the user).
The initChart-saga could also be debounced. Though, I would suspect this to require more changes throughout the codebase and to make testing more complicated.
I assume that english words (average length of five letters) are typed into the input field, and the debounce time is chosen so that the value is only emitted once per word and once per space. Ignoring potential performance benefits to sentry and Redux, this would result in a performance improvement of 41%.
1 - ((5 * (135ms - 67ms) + 135ms) / (5 * 135ms + 135ms))
= 1 - 475ms / 810ms
= 41%
To better benchmark this, I created a very small prototype with an input field and an Highcharts-chart. The input field changes the title of the chart. These benchmarks only focus on the Highcharts performance. There could be additional performance improvements in sentry due to the reduced number of DOM manipulations.
The chart is reinitialized on every key press. The resulting function stack of the Highcharts part is very similar to the one in the everviz editor. The only difference I could spot was an additional call of an onload function at the end that was absent in my prototype.
Nearly all the time (36ms) is spent reinitializing the chart. Interestingly, the average time for the chart to reinitialize was only 33ms. This is only a bit more than half the time it took for the chart in the big everviz editor. I think this could be due to a combination of not using the exact same configuration for the chart, more registered Highcharts-plugins, and additional listeners on DOM elements (sentry) and, in general, a much bigger DOM. Still, I'm not sure this is the whole reason for this difference.
The first improvement was made by reusing the chart instance and calling chart.update() with all chart options. This reduced the time per key press to only 19ms. Most of this time (16ms) was spent again in the rendering of the chart. Notably, there are no destruction calls for the previously instantiated chart (only one destroy() as a result of an update()).
These results can be further improved by only calling the update-function with the changed options. One can compute these changes by comparing the previous options with the current ones. You can see such a function here and tests here. This should behave equivalent to using the update-function with all options, but be more efficient, as Highchart's chart.update() method chooses the parts to update based on the provided options.
In this case, the overhead of checking the difference between the previous and the current options was negligible (0.5ms). Though, in this case, only the title was changed. If more options are changed, this overhead will increase as the isEqual calls will short-circuit less. When observing the DOM, one can see that only the SVG element of the title is changed. The other SVG elements of the charts are not modified.
Again, this optimization improves the performance of the rendering considerably. While the overhead of everything but the chart remained constant at around 4ms, the time spent rendering the chart decreased from 16ms with the naive update method to 6ms. It also seems to separate the changing of the title from the parsing and rendering of the data. This should result in making the chart editor usable with large datasets.
In comparison to the baseline, this change improved the performance of updating the chart from 33ms to 6ms. Assuming a linear scaling between this minimal demonstration and the full everviz chart editor, the time spent to display changed options in the chart should reduce from 67ms to 12ms. This would reduce the total time spent per key press to 80ms, a 40% improvement.
Reusing the chart could also lead to minor functional improvements as changes to the chart-internal state would be preserved. For example, hidden series would stay hidden, the chosen tab in a Stock-chart would be preserved (same for, e.g., drilldown, if supported), and there would not be a "flickering" of the images in the Map/"Pattern fill" example on your website when changing an option.
I believe this could be implemented by only making changes to the initChart-saga in src\pages\ChartEditorPage\middleware\ChartEditor.js. Of course, the chart instance must be cached to be reused in the saga. From my understanding, the Highcharts instance is currently saved to the Redux store for use in other sagas and React components. Reusing an instance would violate the immutability of the Redux state and could lead to synchronization issues in react components, as the selectors do not see a change. I argue that the immutability of the state is already violated by having the Highcharts-instance in it, as Highcharts itself mutates this instance (recursively freezing the instance and interacting with the chart results in errors). In addition, there doesn't seem to be a selector that selects the chart instance directly from the store. Instead, the chart is always retrieved via the getChartConfig selector, which selects the parent object of the chart instance. As the parent object is always recreated when the chart is set via the chartEditor/set reducer, the selector should work as expected, and there should not be any synchronization issues.
Another problem when reusing the chart instance is that undo- and redo-functionality solely based on the store actions would be more complicated to implement, as one could not reset to a previous chart instance. This would probably not work with the current implementation either, as the old chart instance is destroyed when the new chart instance is initialized. Therefore one would have to implement custom code nevertheless to create a new chart instance from the previous state.
At the moment, it also seems like the redrawProject-action is used to reinitialize the chart when the container-HTML-element (.highcharts-container) changes. To preserve this behavior, one could either redraw the chart or force a reinitialization. Currently, the initChart saga also ensures that the chart instance uses the correct container element. Only updating the options of the chart instance would not have this side effect. Instead, one would have to rely on React's reconciliation algorithm to ensure the container element is always reused.
While I believe it is possible to implement this improvement with minimal changes to the implementation, I would recommend refactoring the code to not have the chart instance in the Redux store (it violates the following two essential and one strongly recommended Redux rules: Do not mutate state, Do not put non-serializable values in state or actions, Keep state minimal and derive additional values). Instead, it should be created and managed in the component that renders the .highcharts-container element. This would completely encapsulate the requirement that such a container is always present in the DOM. Currently, one has to call the initChart-saga actively each time a change to the options is made and the redrawProject-saga each time the container element changes. This is error-prone as it could easily be forgotten. Creating the chart instance in the component would leverage the power of the Redux selectors to automatically update the chart when the options change, thereby removing the need for the initChart- and redrawProject-sagas. It would also remove the need for custom chart reinitialization in an undo/redo scenario and restore the immutability of the Redux store's state.
Still, there is the problem of sharing the chart instance between the sagas and the React components. I guess this requirement led to the initial decision to put the chart instance in the Redux store. I believe the most "React"-way to solve this problem would be to use a context or pass it manually between components. However, I think (maybe because of my Angular background) a singleton (service) that provides access to the current instance could be more accessible. The chart rendering component could pass the chart instance to this singleton, which in turn provides global access to this mutable chart instance. However, one must be careful with referencing this chart instance in components. React's change detection would not always notice changes to it, creating state-desynchronization issues between different components. In addition, all changes to chart options should be made via actions to the Redux store instead of directly using this chart instance. On the bright side, this service could explicitly document these limitations. However, this would require a lot of refactoring, and I don't have a good overview of the application's architecture to see all of the problems that would newly arise or reappear with such a refactoring.
In conclusion, these changes could be implemented in a cheap but error-prone or in an expensive and more maintainable way.
I analyzed the everviz-editor and identified the reinitialization of the Highcharts chart to be very performance intensive. I described two aspects that could significantly improve the performance: debouncing key press events and reusing the Highcharts instance. For the simple Line-chart example each of these should improve the performance by around 40%. In combination, they should bring a performance improvement of 48%. For more complex charts, like a Stock-chart, the performance improvement should be even bigger. Reusing the Highcharts chart should also bring additional functional improvements as the chart's internal state changes would be preserved.
1 - ((5 * (80ms - 12ms) + 80ms) / (5 * 135ms + 135ms))
= 1 - 420ms / 810ms
= 48%
I hope this was helpful. I'm looking forward to your feedback.