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

events: improve for-loop #26354

Closed
wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 28, 2019
@lpinca
Copy link
Member

lpinca commented Feb 28, 2019

Is for...of as fast as classic for now? (It probably doesn't matter in this case)

@gengjiawen
Copy link
Member Author

gengjiawen commented Mar 1, 2019

When test count is small (TEST_COUNT = Math.pow(10, 2)), forOf is faster

forLoop: 0.205ms
forOf: 0.107ms

When test count is bigger (TEST_COUNT = Math.pow(10, 8)), forLoop is faster

forLoop: 884.075ms
forOf: 1629.238ms

Env:

  System:
    OS: macOS 10.14.3
    CPU: (8) x64 Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz
    Memory: 1.27 GB / 16.00 GB
    Shell: 3.0.0 - /usr/local/bin/fish
  Binaries:
    Node: 11.10.1 - /usr/local/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 6.7.0 - /usr/local/bin/npm

Test snippet

const TEST_COUNT = Math.pow(10, 2);

const pojo = {
    name: 'n',
    age: 10
}

function Benckmark(desc, func) {
    console.time(desc)
    let sum = 0;
    for (let index = 0; index < TEST_COUNT; index++) {
        func()
    }
    console.timeEnd(desc)
}

const noop = () => {}

function forLoop(o) {
    const keys = Object.keys(o)
    for (let i = 0; i < keys.length; ++i) {	
        const key = keys[i]
        noop(key)
    }
}

function forOf(o) {
    for (const key of Object.keys(o)) {	
        noop(key)
    }
}

Benckmark('forLoop', () => forLoop(pojo))
Benckmark('forOf', () => forOf(pojo))

@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2019

@gengjiawen turning around the execution order is also going to turn around the results for what is faster for big n. Therefore this benchmark does not seem reliable.

The difference between the two loops is not very big but it is ~15%. See https://bugs.chromium.org/p/v8/issues/detail?id=8070 for the tracking bug that I opened last year for it.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 4, 2019

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2019
@BridgeAR BridgeAR changed the title lib: improve for-loop in events.js events: improve for-loop Mar 5, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 5, 2019
PR-URL: nodejs#26354
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Landed in d79176a 🎉

@BridgeAR BridgeAR closed this Mar 5, 2019
@gengjiawen gengjiawen deleted the improve_event_forloop branch March 5, 2019 01:13
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
PR-URL: nodejs#26354
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
PR-URL: #26354
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants