Skip to main content
added 21 characters in body
Source Link
Blindman67
  • 22.3k
  • 2
  • 16
  • 40

TheThere is nothing preventing a new call to setRTO starting a new rAF loop while an existing oneloop is running. This could cause a serious resource drain as more and more loops wait to terminate.

You need to ensure existing loops will terminate when expected, or prevent new loops starting before the current loops haveloop has ended. RewriteThe rewrite (below) uses the semaphore running within DT's closure to prevent more than one loop running at a time.

The is nothing preventing a new call to setRTO starting a new rAF loop while an existing one is running. This could cause a serious resource drain as more and more loops wait to terminate.

You need to ensure existing loops will terminate when expected, or prevent new loops starting before current loops have ended. Rewrite uses semaphore running within DT's closure to prevent more than one loop running at a time.

There is nothing preventing a new call to setRTO starting a new rAF loop while an existing loop is running. This could cause a serious resource drain as more and more loops wait to terminate.

You need to ensure existing loops will terminate when expected, or prevent new loops starting before the current loop has ended. The rewrite (below) uses the semaphore running within DT's closure to prevent more than one loop running at a time.

Source Link
Blindman67
  • 22.3k
  • 2
  • 16
  • 40

I must point out that I can see no usefulness in this code at all. Its acts like a timer with a randomized 16.66...ms error and provides no frame sync at all.

requestAnimationFrame does not sync with display frames, rather it tells the compositor to hold visual changes made in the rAF callback (stored in back buf) until safe to move to display buffer (during VSync).

Most browsers these days also do this with setTimeout and setInterval

Back to the review.

General points

  • Better to implement DT via a factory rather than flat code (see rewrite)

  • Not sure why you define setRTO and RTOLoop outside the object's (DT) declaration.

  • Good to see Object.seal though my preference is to use Object.freeze. This forces the exposed object to act more like an interface, allowing only getters and setters to modify state and ensure object integrity.

  • Avoid indirect calls as they are just a waste of source code characters and CPU cycles. Eg requestAnimationFrame((RAFtimestamp) => DT.RTOloop(RAFtimestamp)); can be requestAnimationFrame(DT.RTOloop);

  • You are not guarding your object's state. callback is not vetted as a function and thus can throw in your code. start and duration are not checked to be numbers and within a valid range.

    Always ensure your object has a valid state.

Undefined unwanted behaviour

The is nothing preventing a new call to setRTO starting a new rAF loop while an existing one is running. This could cause a serious resource drain as more and more loops wait to terminate.

You need to ensure existing loops will terminate when expected, or prevent new loops starting before current loops have ended. Rewrite uses semaphore running within DT's closure to prevent more than one loop running at a time.

Use modern JS.

At first I thought you were aiming for legacy browser support, however you have modern syntax (arrow function in DT.RTOLoop and default parameter in DT.setRTO) which negates all of the old style code, and I question the need to define performance.now, if the browser supports arrow function, performance.now is a given.

Modern JS points.

  • More arrow functions.
  • Object function shorthand declaration. { now(){} } rather than { now: function(){} }
  • ?? rather than ||. Eg performance = window.performance ?? {}

Rewrite

  • Rewrite creates a single instance of DT via a IIFE using a factory pattern.
  • DT is simplified to one property setRTO
  • The behaviour is vetted against invalid parameters and will do nothing if not given good data.
  • Will not start a new timer until existing one (if any) has completed.

const DT = (() => {
    const isNum = val => !isNaN(val) && val < Infinity; //  Special case Infinity is not a number
    var start, duration, callback, running;
    function onFrame(time) {
        time - start < duration ? 
            requestAnimationFrame(onFrame) : 
            (running = false, callback(time, start));
    }
    return Object.freeze({
        setRTO(cb, duration_, start_ = performance.now()) {
            if (cb instanceof Function && isNum(duration_) && isNum(start_) && !running) {
                callback = cb;
                start = start_;
                duration = duration_;
                running = true;
                onFrame(start);
            } else {
                // todo Fail code here
            }
        }
    });
})();


DT.setRTO((stamp, start) => console.log(stamp - start), 500);
DT.setRTO((stamp, start) => console.log(stamp - start), 100);  // this timer will not start