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

process: improve performance of nextTick #8932

Merged
merged 1 commit into from
Oct 13, 2016

Conversation

evanlucas
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process

Description of change

This replaces TickObject with an object literal. This offers
performance improvements of up to ~20%.

Results from a few benchmark runs:

                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2      9.46 %         *** 1.602162e-15
 process/next-tick-breadth.js millions=2          17.76 %         *** 6.210386e-25
 process/next-tick-depth-args.js millions=2        3.19 %         *** 8.427012e-05
 process/next-tick-depth.js millions=2             1.35 %             5.028960e-02
                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2      9.30 %         *** 1.815388e-20
 process/next-tick-breadth.js millions=2          18.25 %         *** 1.806162e-22
 process/next-tick-depth-args.js millions=2        1.57 %             8.013786e-02
 process/next-tick-depth.js millions=2             1.98 %           * 2.573876e-02
                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2      8.56 %         *** 1.609244e-18
 process/next-tick-breadth.js millions=2          19.44 %         *** 1.270555e-25
 process/next-tick-depth-args.js millions=2        2.52 %          ** 2.739874e-03
 process/next-tick-depth.js millions=2             2.11 %          ** 1.289425e-03

/cc @trevnorris since you added TickObject

@evanlucas evanlucas added process Issues and PRs related to the process subsystem. performance Issues and PRs related to the performance of Node.js. labels Oct 4, 2016
@Fishrock123
Copy link
Contributor

Interesting, there is a few places we moved to constructors for previous optimizations, perhaps we should be moving away now?

@evanlucas
Copy link
Contributor Author

@Fishrock123 possibly. Hopefully, I'll be digging some other areas as well this week. I was planning on looking at immediates next, so I'll probably have some questions for you :]

I also need to test on v6.x and see if there are any differences since we updated to V8 5.1

@@ -151,7 +145,11 @@ function setupNextTick() {
args[i - 1] = arguments[i];
}

nextTickQueue.push(new TickObject(callback, args));
nextTickQueue.push({
callback: callback,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, is there a particular reason not to go

nextTickQueue.push({
  args,
  callback,
  domain: process.domain || null
});

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just find it less readable than the way it currently is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer what @claudiorodriguez suggested

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming it does not alter perf that is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't appear to make a difference. Just ran again using shorthand and the numbers are more or less the same. :]

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Oct 5, 2016

Great work. Nice to see that you are using the new benchmarking tool. However you shouldn't do many independent compare.js runs. If you want more certainty just increase the --runs parameter in compare.js. This is not just for convenience, it's also important from a statistical perspective.

For each comparison you do, there is by default a 5% risk that we see a significant result when there in reality no improvement. By doing 3 runs you increase that risk to 3 * 5% = 15% (mathematical inaccurate). On top of this you are doing 4 comparisons with different parameters (nothing wrong here), which increases that risk even more, but that is another story.

@evanlucas
Copy link
Contributor Author

@AndreasMadsen thanks for the info. I ran the bench again once with 100 runs this time and got very similar results:

                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2      9.09 %         *** 1.623609e-39
 process/next-tick-breadth.js millions=2          18.32 %         *** 9.868966e-75
 process/next-tick-depth-args.js millions=2        2.55 %         *** 3.923716e-07
 process/next-tick-depth.js millions=2             2.03 %         *** 1.231088e-06

Should I be doing these runs once for each benchmark script?

@claudiorodriguez
Copy link
Contributor

Copy link
Contributor

@claudiorodriguez claudiorodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Fishrock123
Copy link
Contributor

I'd like to see some info on what branches this can effectively go to before we land it though :)

@evanlucas
Copy link
Contributor Author

@Fishrock123 yea, am going to bench on v6.x now

@jbergstroem
Copy link
Member

(pending checks are a bug -- i need to cover skipped status)

@evanlucas
Copy link
Contributor Author

results for v6.x:

                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2     15.16 %         *** 3.828618e-92
 process/next-tick-breadth.js millions=2          10.47 %         *** 2.817494e-51
 process/next-tick-depth-args.js millions=2        1.38 %         *** 6.977638e-04
 process/next-tick-depth.js millions=2             1.73 %         *** 6.359614e-04

@AndreasMadsen
Copy link
Member

@evanlucas Should I be doing these runs once for each benchmark script?

Not sure what you mean. But it looks like you did it correctly 👍

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imyller
Copy link
Member

imyller commented Oct 5, 2016

Oh, what I meant to say is LATM. Looks Awesome To Me.

@jbergstroem
Copy link
Member

LGTM

@evanlucas
Copy link
Contributor Author

Running CI one more time since I updated it: https://ci.nodejs.org/job/node-test-pull-request/4403/

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@trevnorris
Copy link
Contributor

LGTM. Though we shouldn't put much on the breadth tests. 99% of the time the nextTickQueue array has fewer than 5 callbacks when processed.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@evanlucas
Copy link
Contributor Author

I am going to benchmark on v4.x tonight to see if it is even worth it there as well.

@evanlucas
Copy link
Contributor Author

So, for v4, I had to use the old benchmarks. It doesn't seem to make as much of a difference as on v6.

running ./node_after_v4
process/next-tick-breadth-args.js
process/next-tick-breadth.js
process/next-tick-depth-args.js
process/next-tick-depth.js
running ./node_before_v4
process/next-tick-breadth-args.js
process/next-tick-breadth.js
process/next-tick-depth-args.js
process/next-tick-depth.js

process/next-tick-breadth-args.js millions=2: ./node_after_v4: 1.9412 ./node_before_v4: 1.7772 . 9.23%
process/next-tick-breadth.js millions=2: ./node_after_v4: 3.1019 ./node_before_v4: 3.1364 ..... -1.10%
process/next-tick-depth-args.js millions=2: ./node_after_v4: 10.658 ./node_before_v4: 10.308 ... 3.40%
process/next-tick-depth.js millions=2: ./node_after_v4: 18.469 ./node_before_v4: 18.68 ........ -1.13%

so, we can probably just not backport this to v4.x

@AndreasMadsen
Copy link
Member

@evanlucas you are supposed to be able to use the new benchmarking suite on old node versions. The process is:

git checkout upstream/v4.x-staging # or similar branch
make -j8
cp ./node ./node-v4

apply-pr 8932 # fix the file missing conflict
make -j8
cp ./node ./node-v4-pr8932

git checkout master
make -j4 # or just use your globally installed version

./node ./benchmark/compare.js \
  --old ./node-v4 --new ./node-v4-pr8932 \
  --filter next-tick -- process | tee results.csv
cat results.csv | Rscript ./benchmark/compare.R

Unfortunately some new syntax has sneaked into common.js. Replacing ...args with args = Array.prototype.slice.call(arguments, 1); fixes it (easy first contribution anyone!).

This gives the result (30 runs):

                                              improvement significant      p.value
 process/next-tick-breadth-args.js millions=2     12.60 %         *** 4.418516e-28
 process/next-tick-breadth.js millions=2           4.47 %         *** 5.463876e-06
 process/next-tick-depth-args.js millions=2       -1.01 %             4.254800e-01
 process/next-tick-depth.js millions=2             3.30 %           * 2.142562e-02

@AndreasMadsen AndreasMadsen mentioned this pull request Oct 12, 2016
2 tasks
This replaces TickObject with an object literal. This offers
performance improvements of up to ~20%.

PR-URL: nodejs#8932
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@evanlucas evanlucas deleted the nexttickperf branch October 13, 2016 17:18
@evanlucas
Copy link
Contributor Author

Landed in 804d57d. Thanks!

@evanlucas evanlucas merged commit 804d57d into nodejs:master Oct 13, 2016
jasnell pushed a commit that referenced this pull request Oct 14, 2016
This replaces TickObject with an object literal. This offers
performance improvements of up to ~20%.

PR-URL: #8932
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
Using new syntax such as `...args` means that the benchmark suite can't
be used with older node versions. This changes the `...args` syntax to a
good old `Array.prototype.slice`.

Refs: #8932 (comment)
PR-URL: #9064
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this pull request Oct 17, 2016
Using new syntax such as `...args` means that the benchmark suite can't
be used with older node versions. This changes the `...args` syntax to a
good old `Array.prototype.slice`.

Refs: #8932 (comment)
PR-URL: #9064
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins
Copy link
Contributor

@evanlucas how long should we be letting this live on v7 before backporting?

@evanlucas
Copy link
Contributor Author

It's a pretty small change so it doesn't really matter to me. I think it should be fine to get in to the next v6.x release if no one objects

@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS v4.x. Added dont-land label. Please feel free to manually backport

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This replaces TickObject with an object literal. This offers
performance improvements of up to ~20%.

PR-URL: #8932
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This replaces TickObject with an object literal. This offers
performance improvements of up to ~20%.

PR-URL: #8932
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
@evanlucas
Copy link
Contributor Author

Backport for v4.x can be found at #10433. Thanks!

MylesBorins added a commit that referenced this pull request Jan 3, 2017
This LTS release comes with 312 commits. This includes 229 that are
test related, 62 that are docs related, 17 which are build / tools
related, and 4 commits which are updates to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* deps:
    - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
             #9847
    - *V8*: Destructuring of arrow function arguments via computed
            property no longer throws (Michaël Zasso)
            #10386)
* inspector:
  - /json/version returns object, not an object wrapped in an array
    (Ben Noordhuis) #9762
* module:
  - using --debug-brk and --eval together now works as expected
    (Kelvin Jin) #8876
* process:
  - improve performance of nextTick up to 20% (Evan Lucas)
    #8932
* repl:
    - the division operator will no longer be accidentally parsed as
      regex (Teddy Katz) #10103
    - improved support for generator functions (Teddy Katz)
      #9852
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10394
MylesBorins added a commit that referenced this pull request Jan 3, 2017
This LTS release comes with 312 commits. This includes 229 that are
test related, 62 that are docs related, 17 which are build / tools
related, and 4 commits which are updates to dependencies.

Notable Changes:

* build:
  - shared library support is now working for AIX builds
    (Stewart Addison) #9675
* deps:
    - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
             #9847
    - *V8*: Destructuring of arrow function arguments via computed
            property no longer throws (Michaël Zasso)
            #10386)
* inspector:
  - /json/version returns object, not an object wrapped in an array
    (Ben Noordhuis) #9762
* module:
  - using --debug-brk and --eval together now works as expected
    (Kelvin Jin) #8876
* process:
  - improve performance of nextTick up to 20% (Evan Lucas)
    #8932
* repl:
    - the division operator will no longer be accidentally parsed as
      regex (Teddy Katz) #10103
    - improved support for generator functions (Teddy Katz)
      #9852
* timers:
  - Re canceling a cancelled timers will no longer throw
    (Jeremiah Senkpiel) #9685

PR-URL: #10394
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    This LTS release comes with 312 commits. This includes 229 that are
    test related, 62 that are docs related, 17 which are build / tools
    related, and 4 commits which are updates to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * deps:
        - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
                 nodejs/node#9847
        - *V8*: Destructuring of arrow function arguments via computed
                property no longer throws (Michaël Zasso)
                nodejs/node#10386)
    * inspector:
      - /json/version returns object, not an object wrapped in an array
        (Ben Noordhuis) nodejs/node#9762
    * module:
      - using --debug-brk and --eval together now works as expected
        (Kelvin Jin) nodejs/node#8876
    * process:
      - improve performance of nextTick up to 20% (Evan Lucas)
        nodejs/node#8932
    * repl:
        - the division operator will no longer be accidentally parsed as
          regex (Teddy Katz) nodejs/node#10103
        - improved support for generator functions (Teddy Katz)
          nodejs/node#9852
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10394

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    This LTS release comes with 312 commits. This includes 229 that are
    test related, 62 that are docs related, 17 which are build / tools
    related, and 4 commits which are updates to dependencies.

    Notable Changes:

    * build:
      - shared library support is now working for AIX builds
        (Stewart Addison) nodejs/node#9675
    * deps:
        - *npm*: upgrade npm to 3.10.10 (Rebecca Turner)
                 nodejs/node#9847
        - *V8*: Destructuring of arrow function arguments via computed
                property no longer throws (Michaël Zasso)
                nodejs/node#10386)
    * inspector:
      - /json/version returns object, not an object wrapped in an array
        (Ben Noordhuis) nodejs/node#9762
    * module:
      - using --debug-brk and --eval together now works as expected
        (Kelvin Jin) nodejs/node#8876
    * process:
      - improve performance of nextTick up to 20% (Evan Lucas)
        nodejs/node#8932
    * repl:
        - the division operator will no longer be accidentally parsed as
          regex (Teddy Katz) nodejs/node#10103
        - improved support for generator functions (Teddy Katz)
          nodejs/node#9852
    * timers:
      - Re canceling a cancelled timers will no longer throw
        (Jeremiah Senkpiel) nodejs/node#9685

    PR-URL: nodejs/node#10394

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.