mini-runner icon indicating copy to clipboard operation
mini-runner copied to clipboard

Priorities

Open drkibitz opened this issue 8 years ago • 3 comments
trafficstars

Hello @GoodBoyDigital. @ivanpopelyshev pointed me in the direction of this repo from this thread: https://github.com/pixijs/pixi.js/pull/3570

Please see my comment here https://github.com/pixijs/pixi.js/pull/3570#issuecomment-287529747 as this is a followup to that, just transferred to this repo.


This is any interesting approach for sure, but I would probably say the same thing I did in that comment, as this is optimizing for a particular feature of convenience, though doing a nice job of it, being that the whole concept is to allow multiple optional arguments. I wonder if you find it is needed, or if it can just handle one argument like oldskool event apis?

Also wondering if we can converge on something here with a priority-based api?

Here is a Gist of what I have for the ticker stuff I mentioned: https://gist.github.com/drkibitz/79df1f458aa0e0db02eed338de1a25f8

Things to note:

  • Priority is only used for insertion (searches from start and end)
  • Removal is simply a filter
  • Dispatch just takes a single optional argument.

drkibitz avatar Mar 18 '17 18:03 drkibitz

To converge on the priority stuff, I'm seeing rather quickly that we would need to wrap your items in a listener object. Nothing to do with scoping the functions, it is just to have a higher level object to hold the reference, with description properties next to it, e.g. priority.

Here's what I'm thinking (note the changes to the loop in generateRun):

p.add = function(item, priority) {
    if (!item[this._name]) return;

    this.remove(item);

    const listenersCopy = this.listeners.slice();
    const l = listenersCopy.length;
    const listener = { item, priority };

    if (l > 0) {
        let i = l;

        while (i--) {
            // from end - if next has lower priority, insert now after
            if (listenersCopy[i].priority < listener.priority) {
                listenersCopy.splice(i + 1, 0, listener);
                break;
            }
            // from start - if next has equal priority, insert now before
            else if (listenersCopy[l - (i + 1)].priority === listener.priority) {
                listenersCopy.splice(l - (i + 1), 0, listener);
                break;
            }
        }
        this.listeners = listenersCopy;
    }  else {
        listenersCopy.push(listener);
    }
    this.listeners = listenersCopy;
};

MiniRunner.generateRun = function(name, argsLength) {
    const key = name + '|' + argsLength;
    let func = MiniRunner.hash[key];

    if (!func) {
        if (argsLength > 0) {
            let args = 'arg0';

            for(let i = 1; i < argsLength; i++) {
                args += ',arg'+i;
            }
            func = new Function(args, 'var listeners = this.listeners, i = listeners.length; while(i--) listeners[i].item.'+name+'('+args+');');
        } else {
            func = new Function(args, 'var listeners = this.listeners, i = listeners.length; while(i--) listeners[i].item.'+name+'();');
        }

        MiniRunner.hash[key] = func;
    }

    return func;
};

drkibitz avatar Mar 18 '17 18:03 drkibitz

@GoodBoyDigital so I made some changes locally, and I'm seeing something odd, too good to be true even. Take a look at this:

benchmarking signals...
signals time 1.129935
listener updates 2000
benchmarking mini-signals...
mini-signals time 0.1500250000000001
listener updates 4000
benchmarking events...
events time 0.08157999999999993
listener updates 6000
benchmarking runner...
runner time 0.00013999999999987268
listener updates 8000
runner is 8070.964285721625x faster than signals
runner is 1071.607142858118x faster than mini-signals
runner is 582.7142857148152x faster than events

This is consistent... compared to the 25x, 2x, 1x average without my changes.

I can't believe my changes would have increased performance 500%, my expectation was that it might have dropped it by like 0.1% or something. I added the listener updates # log to make sure things were being called. I'm kind of at a loss here. See anything out of the ordinary, or curious? Make a PR? ;)

drkibitz avatar Mar 19 '17 02:03 drkibitz

Nevermind I found the issue, definitely too good to be true.

drkibitz avatar Mar 19 '17 05:03 drkibitz