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

After adding "pointIntervalUnit" to column chart, highcharts crashes tab with out of memory exception. #6405

Closed
AntanasBarauskas opened this Issue Feb 23, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@AntanasBarauskas

AntanasBarauskas commented Feb 23, 2017

Expected behaviour

After adding pointIntervalUnit property to plotOptions.series, browser tab results in out of memory exception. This behavior occurs in Chrome/FF/IE. Added jsFiddle example with crashing code.

Actual behaviour

Crashes with out of memory exception.
NOTE: Changing "type":"datetime" results in freezing tab instead of out of memory exception.

Live demo with steps to reproduce

http://jsfiddle.net/720aboww/

Affected browser(s)

Chrome/FF/IE

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Feb 23, 2017

Contributor

Hi @AntanasBarauskas

Thank you for this report. Could you post also a demo which doesn't crash the browser? It's a bit hard to debug.. :)

Thanks!

Contributor

pawelfus commented Feb 23, 2017

Hi @AntanasBarauskas

Thank you for this report. Could you post also a demo which doesn't crash the browser? It's a bit hard to debug.. :)

Thanks!

@AntanasBarauskas

This comment has been minimized.

Show comment
Hide comment

AntanasBarauskas commented Feb 23, 2017

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Feb 23, 2017

Contributor

Thanks!

The problem is setting xAxis.categories. That means Highcharts will try to render every category (tickInterval = 1unit, for a datetime axis unit is 1ms) for a given range. So, if you set pointerInveralUnit to one year, it means ~365 * 24 * 360 * 1000 * number_of_points categories on xAxis.. :)

After removing xAxis.categories, it works fine: https://jsfiddle.net/06b4kvge/2/

Contributor

pawelfus commented Feb 23, 2017

Thanks!

The problem is setting xAxis.categories. That means Highcharts will try to render every category (tickInterval = 1unit, for a datetime axis unit is 1ms) for a given range. So, if you set pointerInveralUnit to one year, it means ~365 * 24 * 360 * 1000 * number_of_points categories on xAxis.. :)

After removing xAxis.categories, it works fine: https://jsfiddle.net/06b4kvge/2/

@AntanasBarauskas

This comment has been minimized.

Show comment
Hide comment
@AntanasBarauskas

AntanasBarauskas Feb 23, 2017

You're right, removed xAxis.categories and works like a charm!

Thanks.

AntanasBarauskas commented Feb 23, 2017

You're right, removed xAxis.categories and works like a charm!

Thanks.

@inoyakaigor

This comment has been minimized.

Show comment
Hide comment
@inoyakaigor

inoyakaigor Sep 11, 2018

@pawelfus Hi!
Yesterday I've got an out of memory error. I had wrong config (with xAxis.categories) and in this line browser throw exception.

tickPositions.push(t);

It was happen because, I believe, in tickPositions array had been containing ≈9 500 000 000 time numbers (difference between 01.05.2017 and 01.09.2018 in milliseconds). Of course it is my mistake but if Highcharts notified me about potentially memory leak I'd save a lot of time.

Therefore I suggest to add warning in this line about big length of tickPositions array. As sample if length more than 10 000 000

inoyakaigor commented Sep 11, 2018

@pawelfus Hi!
Yesterday I've got an out of memory error. I had wrong config (with xAxis.categories) and in this line browser throw exception.

tickPositions.push(t);

It was happen because, I believe, in tickPositions array had been containing ≈9 500 000 000 time numbers (difference between 01.05.2017 and 01.09.2018 in milliseconds). Of course it is my mistake but if Highcharts notified me about potentially memory leak I'd save a lot of time.

Therefore I suggest to add warning in this line about big length of tickPositions array. As sample if length more than 10 000 000

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 12, 2018

Contributor

Hi @inoyakaigor

Thanks you for the suggestion! We used to throw Highcharts Error 19 in v4.0.x (link: https://www.highcharts.com/errors/19). I'm not sure about the story behind removing this, probably users preferred removing additional ticks instead of the error.

@TorsteinHonsi
bisect leads to db6981e#diff-ca66923e4c3df841c40471c133bce754 (search for factor19) - anything to add here?

Contributor

pawelfus commented Sep 12, 2018

Hi @inoyakaigor

Thanks you for the suggestion! We used to throw Highcharts Error 19 in v4.0.x (link: https://www.highcharts.com/errors/19). I'm not sure about the story behind removing this, probably users preferred removing additional ticks instead of the error.

@TorsteinHonsi
bisect leads to db6981e#diff-ca66923e4c3df841c40471c133bce754 (search for factor19) - anything to add here?

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 13, 2018

Collaborator

anything to add here?

Yes, I thought error 19 was no longer necessary after this refactoring. By default, categories will be decimated: http://jsfiddle.net/highcharts/arxqgy5e/.

I'll look into the issue.

Collaborator

TorsteinHonsi commented Sep 13, 2018

anything to add here?

Yes, I thought error 19 was no longer necessary after this refactoring. By default, categories will be decimated: http://jsfiddle.net/highcharts/arxqgy5e/.

I'll look into the issue.

@TorsteinHonsi TorsteinHonsi self-assigned this Sep 13, 2018

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 13, 2018

Collaborator

I'm not able to reproduce it from the description - can you make changes to http://jsfiddle.net/highcharts/arxqgy5e/5/ in order to showcase the issue?

Collaborator

TorsteinHonsi commented Sep 13, 2018

I'm not able to reproduce it from the description - can you make changes to http://jsfiddle.net/highcharts/arxqgy5e/5/ in order to showcase the issue?

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 13, 2018

Contributor

@inoyakaigor - could you update the demo posted by Torstein?

Contributor

pawelfus commented Sep 13, 2018

@inoyakaigor - could you update the demo posted by Torstein?

@inoyakaigor

This comment has been minimized.

Show comment
Hide comment
@inoyakaigor

inoyakaigor Sep 13, 2018

@pawelfus yeah I will try but later

inoyakaigor commented Sep 13, 2018

@pawelfus yeah I will try but later

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 13, 2018

Contributor

Can be related to #8026

Contributor

pawelfus commented Sep 13, 2018

Can be related to #8026

@inoyakaigor

This comment has been minimized.

Show comment
Hide comment
@inoyakaigor

inoyakaigor Sep 13, 2018

http://jsfiddle.net/d9gu1eoj/5/ @pawelfus @TorsteinHonsi

Chrome silent disconnect a devtools. Firefox write in console out of memory error
Mistake in xAxis.labels.step = 1

inoyakaigor commented Sep 13, 2018

http://jsfiddle.net/d9gu1eoj/5/ @pawelfus @TorsteinHonsi

Chrome silent disconnect a devtools. Firefox write in console out of memory error
Mistake in xAxis.labels.step = 1

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 14, 2018

Contributor

Thank you @inoyakaigor for the demo!

Simplified demo: http://jsfiddle.net/BlackLabel/twf2kqr9/ (or based on Torstein's demo: http://jsfiddle.net/BlackLabel/arxqgy5e/14/ )
Actually, removing type: "datetime" does not resolve the issue: http://jsfiddle.net/BlackLabel/twf2kqr9/2/

@TorsteinHonsi - how should we resolve this? Prevent categories to display when datetime type is set? But this is misconfigured on a label's level.

Contributor

pawelfus commented Sep 14, 2018

Thank you @inoyakaigor for the demo!

Simplified demo: http://jsfiddle.net/BlackLabel/twf2kqr9/ (or based on Torstein's demo: http://jsfiddle.net/BlackLabel/arxqgy5e/14/ )
Actually, removing type: "datetime" does not resolve the issue: http://jsfiddle.net/BlackLabel/twf2kqr9/2/

@TorsteinHonsi - how should we resolve this? Prevent categories to display when datetime type is set? But this is misconfigured on a label's level.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 14, 2018

Collaborator

It also fails when the type is not datetime, except it doesn't crash immediately. Maybe we need to reintroduce error 19 for cases when label.step is set?

Collaborator

TorsteinHonsi commented Sep 14, 2018

It also fails when the type is not datetime, except it doesn't crash immediately. Maybe we need to reintroduce error 19 for cases when label.step is set?

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Sep 14, 2018

Contributor

I think solution suggested by @inoyakaigor is super easy: simply when we have more than X ticks, show warning/error ?

Contributor

pawelfus commented Sep 14, 2018

I think solution suggested by @inoyakaigor is super easy: simply when we have more than X ticks, show warning/error ?

@inoyakaigor

This comment has been minimized.

Show comment
Hide comment
@inoyakaigor

inoyakaigor Sep 14, 2018

I think a warning should be shown if the number of ticks is greater than the width of the graph in pixels.

inoyakaigor commented Sep 14, 2018

I think a warning should be shown if the number of ticks is greater than the width of the graph in pixels.

TorsteinHonsi added a commit that referenced this issue Sep 17, 2018

Fixed #6405, Highcharts crashed when using category axis, small label…
…s.step option and a large data range. Re-introduced error 19.
@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 17, 2018

Collaborator

Done. I re-introduced the old logic that fires when the potential tick-interval is more than makes sense, but unlike before, added a more graceful degradation. The axis now render two ticks (min and max) instead of crashing the whole chart, and print a warning in the console. http://jsfiddle.net/highcharts/twf2kqr9/12/

Collaborator

TorsteinHonsi commented Sep 17, 2018

Done. I re-introduced the old logic that fires when the potential tick-interval is more than makes sense, but unlike before, added a more graceful degradation. The axis now render two ticks (min and max) instead of crashing the whole chart, and print a warning in the console. http://jsfiddle.net/highcharts/twf2kqr9/12/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment