Opened 9 years ago
Closed 8 years ago
#15589 closed enhancement (fixed)
avoid using setInterval since it can back up
Reported by: | ben hockey | Owned by: | cjolif |
---|---|---|---|
Priority: | undecided | Milestone: | 1.9 |
Component: | Charting | Version: | 1.7.3 |
Keywords: | Cc: | cjolif | |
Blocked By: | Blocking: |
Description
using setInterval, it's possible to queue up new "tasks" before the previous ones have finished. by using setTimeout in the handler itself to start it over again, you can avoid this problem and slightly simplify some other logic.
-
charting/action2d/_IndicatorElement.js
diff --git a/charting/action2d/_IndicatorElement.js b/charting/action2d/_IndicatorElement.js index 8967ec3..6ee92aa 100644
a b define(["dojo/_base/lang", "dojo/_base/declare", "../plot2d/Base", "../plot2d/co 88 88 _trackMove: function(){ 89 89 // let's update the selector 90 90 this._updateIndicator(this.pageCoord); 91 // if we reached that point once, then we don't stop until mouse up 92 if(this._initTrackPhase){ 93 this._initTrackPhase = false; 94 this._tracker = setInterval(lang.hitch(this, this._trackMove), 100); 95 } 91 // use a recursive setTimeout to avoid intervals that might get backed up 92 this._tracker = setTimeout(lang.hitch(this, this._trackMove), 100); 96 93 }, 97 94 initTrack: function(){ 98 this._initTrackPhase = true;95 if(!this._tracker){ 99 96 this._tracker = setTimeout(lang.hitch(this, this._trackMove), 500); 97 } 100 98 }, 101 99 stopTrack: function(){ 102 100 if(this._tracker){ 103 if(this._initTrackPhase){104 101 clearTimeout(this._tracker); 105 }else{106 clearInterval(this._tracker);107 }108 102 this._tracker = null; 109 103 } 110 104 },
Change History (8)
comment:1 Changed 9 years ago by
Cc: | cjolif added |
---|
comment:2 Changed 9 years ago by
This seems to work fine on desktop I would need to pass all the mobile devices tests before committing this. To gauge the priority of this did you experience actual issues with setInterval here? If not it sounds more like a code enhancement/clean up than a defect?
comment:3 Changed 9 years ago by
Type: | defect → enhancement |
---|
we are doing real time data updates (potentially with frequency in the range of 10s of ms) that get conflated on the client (the chart is fed a single data point to represent maybe 20 of the real data points) and have noticed a more general performance issue with memory leaks and performance degradation in general with the interactions between our code and dojox/charting.
i have not been able to pinpoint this particular code as a specific issue but i'm on a hunt for anything that looks suspicious and a setInterval
of 100ms is a red flag since if there is a stress on the browser, it's possible that these could start to back up and just compound problems further.
i'm happy to switch it to an enhancement.
while looking at this, i also saw ways to improve MouseIndicator? that leverages this change. if i find some time i'll see if i can ever get a patch with those improvements... but it's really low priority for me because i've built my own indicator similar to MouseIndicator? but don't actually use MouseIndicator?.
comment:4 Changed 9 years ago by
ok I will try to get the testing done, that said the interval/timer should fire only during the interaction so it does not sound to me like something that should create a leaking in the long term when updating data regularly. I've done a few patches on the memory leak side recently, don't know if you applied them (they should be in 1.7.3)? If not you should definitely.
For MouseIndicator I'm curious to know why you needed your own version? Missing features?
comment:5 Changed 9 years ago by
we are using trunk and we have seen some improvements so that's helping - thanks.
as for MouseIndicator?, we wanted something like http://www.google.com/finance?q=INDEXDJX:.DJI which follows the mouse on hover rather than based on clicking and dragging. i can see that click/touch and drag is more suited to mobile but hover is a better experience for desktop and is missing from MouseIndicator?. it would probably be possible to have the handling for click and hover in a single Indicator but i'm currently just doing a single Indicator with the hover logic only. for mobile we're just not supporting that feature yet.
comment:6 Changed 9 years ago by
Thanks for the information. For MouseIndicator I guess this could be an option to make it work on hover as well.
comment:7 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|---|
Owner: | changed from Eugene Lazutkin to cjolif |
Status: | new → assigned |
please check this thoroughly before applying it. i've done some smoke tests but another look at it would help.