Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Priorities #4

Open
drkibitz opened this issue Mar 18, 2017 · 3 comments
Open

Priorities #4

drkibitz opened this issue Mar 18, 2017 · 3 comments

Comments

@drkibitz
Copy link

drkibitz commented Mar 18, 2017

Hello @GoodBoyDigital. @ivanpopelyshev pointed me in the direction of this repo from this thread:
pixijs/pixijs#3570

Please see my comment here pixijs/pixijs#3570 (comment)
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
Copy link
Author

drkibitz commented Mar 18, 2017

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
Copy link
Author

drkibitz commented Mar 19, 2017

@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 added a commit to drkibitz/mini-runner that referenced this issue Mar 19, 2017
@drkibitz
Copy link
Author

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

drkibitz added a commit to drkibitz/mini-runner that referenced this issue Mar 19, 2017
commit c8118d7
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 22:45:51 2017 -0700

    Lets try this again

commit 7400369
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 22:17:51 2017 -0700

    Fix remove - more doc and cleanup.

    - The remove was causing the bad benchmarking, filtering was not working. This matches the master benchmarks .

commit 9a7bccd
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:25:42 2017 -0700

    lint

commit 695369b
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:24:22 2017 -0700

    General cleanup - docs, and named functions

commit dc1c33a
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:14:28 2017 -0700

    GoodBoyDigital#4 add priority to add/remove methods

    - tweaks to benchmark script
drkibitz added a commit to drkibitz/mini-runner that referenced this issue Mar 19, 2017
commit c8118d7
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 22:45:51 2017 -0700

    Lets try this again

commit 7400369
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 22:17:51 2017 -0700

    Fix remove - more doc and cleanup.

    - The remove was causing the bad benchmarking, filtering was not working. This matches the master benchmarks .

commit 9a7bccd
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:25:42 2017 -0700

    lint

commit 695369b
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:24:22 2017 -0700

    General cleanup - docs, and named functions

commit dc1c33a
Author: Dr. Kibitz <info@drkibitz.com>
Date:   Sat Mar 18 20:14:28 2017 -0700

    GoodBoyDigital#4 add priority to add/remove methods

    - tweaks to benchmark script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant