Opened 7 years ago

Closed 7 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 
    8888       _trackMove: function(){
    8989           // let's update the selector
    9090           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);
    9693       },
    9794       initTrack: function(){
    98            this._initTrackPhase = true;
     95           if(!this._tracker){
    9996               this._tracker = setTimeout(lang.hitch(this, this._trackMove), 500);
     97           }
    10098       },
    10199       stopTrack: function(){
    102100           if(this._tracker){
    103                if(this._initTrackPhase){
    104101               clearTimeout(this._tracker);
    105                }else{
    106                    clearInterval(this._tracker);
    107                }
    108102               this._tracker = null;
    109103           }
    110104       },

Change History (8)

comment:1 Changed 7 years ago by ben hockey

Cc: cjolif added

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

comment:2 Changed 7 years ago by 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?

comment:3 Changed 7 years ago by ben hockey

Type: defectenhancement

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

Last edited 7 years ago by ben hockey (previous) (diff)

comment:4 Changed 7 years ago by 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?

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by cjolif

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

comment:7 Changed 7 years ago by cjolif

Milestone: tbd1.9
Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

comment:8 Changed 7 years ago by cjolif

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.