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.

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

Multiple redraw events fired during annotations line drawing #17582

Closed
kamil-musialowski opened this issue Aug 8, 2022 · 3 comments
Closed
Labels
Type: Feature Request Used when a new feature is requested either directly or indirectly

Comments

@kamil-musialowski
Copy link
Contributor

kamil-musialowski commented Aug 8, 2022

Description of the feature

Function responsible for creating an annotation could be improved.

For example, redraw event is fired on every mouse move during line drawing which is very performance heavy (the same issue is not happening when updating or dragging a line).
Is there a point to redraw the whole chart during that?

Library related to the feature

Highcharts Stock with Stock Tools GUI

Proof of Concept/Live example for the feature

DEMO: https://jsfiddle.net/BlackLabel/rdLu86z4/

  1. Open up the console.
  2. Click the 3rd option from the top (draw a line).
  3. Click the mouse on the chart once, to create a starting point.
  4. Move mouse around and watch how many redraw events are called.

Also, watch the console while updating the same line.


You can vote for this feature by adding a thumbs-up reaction to this post.

@kamil-musialowski kamil-musialowski added the Type: Feature Request Used when a new feature is requested either directly or indirectly label Aug 8, 2022
@pawelfus
Copy link
Contributor

pawelfus commented Aug 8, 2022

Workaround:
Replace chart.redraw() with chart.drawAnnotations() in the Annotation.update() method.
Demo: https://jsfiddle.net/BlackLabel/rdLu86z4/1/
Snippet:

Highcharts.Annotation.prototype.update = function(userOptions, redraw) {
  var chart = this.chart,
    labelsAndShapes = this.getLabelsAndShapesOptions(this.userOptions, userOptions),
    userOptionsIndex = chart.annotations.indexOf(this),
    options = Highcharts.merge(true, this.userOptions, userOptions);
  options.labels = labelsAndShapes.labels;
  options.shapes = labelsAndShapes.shapes;
  this.destroy();
  this.constructor(chart, options);
  // Update options in chart options, used in exporting (#9767):
  chart.options.annotations[userOptionsIndex] = options;
  this.isUpdating = true;
  if (Highcharts.pick(redraw, true)) {
    chart.drawAnnotations();
  }
  Highcharts.fireEvent(this, 'afterUpdate');
  this.isUpdating = false;
};

Internal note:
In fact, we should fix performance issues here:

  1. Unnecessary grouped data destruction. Regression after Lazy loading multiple series causes various bugs in highcharts and highstock functions #10290
    Zrzut ekranu 2022-08-8 o 14 18 15
    It seems the condition is wrong, forcing the chart to recalculate series in the navigator in each redraw(). The condition in afterSetScale should be more like this: https://jsfiddle.net/BlackLabel/rdLu86z4/3/ (drops the time from ~60ms to ~30ms)

  2. RangeSelector.render() is called twice (and alignItems() is called three times, ~8ms). Disabling RangeSelector gives < 20ms per frame
    Zrzut ekranu 2022-08-8 o 14 28 12

  3. a11y module takes ~15ms to describe the series. @oysteinmoseng do you have any ideas on how could we improve chart.redraw()? So describeSeries() will for example skip its logic when nothing has changed? Simplified demo: https://jsfiddle.net/BlackLabel/rdLu86z4/4/
    Steps:

  • open the performance tab and start recording
  • click on a button
  • stop recording, check the results:
    Zrzut ekranu 2022-08-8 o 14 57 18
  1. With fix, disabled RangeSelector, disabled a11y, we get chart.redraw() below 5ms:
    Zrzut ekranu 2022-08-8 o 14 38 54
    Which consists of (when creating an annotation):
  • updating points shape (setting state): ~1ms
  • calculating axis margins (not sure why): ~1ms
  • redrawing navigator: <0.5s
  • rendering annotation: ~1-2ms

@oysteinmoseng
Copy link
Member

@pawelfus Many ways to optimize a11y descriptions, as you say we could keep track of whether or not changes have been made. Also with data grouping I would have thought it was already pretty fast, so we might want to look into which parts specifically are taking long.

@pawellysy
Copy link
Member

The original Issue was fixed by #20080, and the respective comments of @pawelfus will be addressed here:
#20226
#20217
#20214
#20207

Closing as completed.

Development-Flow automation moved this from To do to Done Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Request Used when a new feature is requested either directly or indirectly
Projects
Development

No branches or pull requests

5 participants