avoid using setInterval since it can back up

[ben hockey]

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

@@ -88 +88 @@
        _trackMove: function(){
            // let's update the selector
            this._updateIndicator(this.pageCoord);
-           // if we reached that point once, then we don't stop until mouse up
-           if(this._initTrackPhase){
-               this._initTrackPhase = false;
-               this._tracker = setInterval(lang.hitch(this, this._trackMove), 100);
-           }
+           // use a recursive setTimeout to avoid intervals that might get backed up
+           this._tracker = setTimeout(lang.hitch(this, this._trackMove), 100);
        },
        initTrack: function(){
-           this._initTrackPhase = true;
+           if(!this._tracker){
                this._tracker = setTimeout(lang.hitch(this, this._trackMove), 500);
+           }
        },
        stopTrack: function(){
            if(this._tracker){
-               if(this._initTrackPhase){
                clearTimeout(this._tracker);
-               }else{
-                   clearInterval(this._tracker);
-               }
                this._tracker = null;
            }
        },

[ben hockey]

please check this thoroughly before applying it. i've done some smoke tests but another look at it would help.

[cjolif]

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?

[ben hockey]

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?.

[cjolif]

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?

[ben hockey]

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.

[cjolif]

Thanks for the information. For MouseIndicator I guess this could be an option to make it work on hover as well.

[cjolif]

In [29694]:

fixes #15486, #15589. Implements a plot threshold "indicator" that can be drawn vertically or horizontally on the chart. Make sure that the mouse/touch indicator is leveraging it. Include some small enhancements to mouse indicator (on"over") as well as some CartesianBase? code refactoring. !strict.