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: performance regression in event benchmarks #12657

Closed
jasnell opened this issue Apr 25, 2017 · 9 comments
Closed

events: performance regression in event benchmarks #12657

jasnell opened this issue Apr 25, 2017 · 9 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js.

Comments

@jasnell
Copy link
Member

jasnell commented Apr 25, 2017

See: #12655 (comment)

$ ./node benchmark/compare.js --old node --new ../main/node events > ~/eventsbench
[00:10:21|% 100| 7/7 files | 60/60 runs | 1/1 configs]: Done
 james@ubuntu:~/node/node$cat ~/eventsbench | Rscript benchmark/compare.Rh
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -45.73 %        *** 8.380381e-18
 events/ee-emit-multi-args.js n=2000000                   0.70 %            2.285963e-01
 events/ee-emit.js n=2000000                              2.12 %        *** 3.829377e-05
 events/ee-listener-count-on-prototype.js n=50000000    -95.07 %        *** 1.717885e-35
 events/ee-listeners-many.js n=5000000                  -20.60 %        *** 8.616063e-14
 events/ee-listeners.js n=5000000                       -48.69 %        *** 2.759171e-60
 events/ee-once.js n=20000000                           -58.16 %        *** 3.019260e-56

I'm seeing a significant performance regression in the events benchmarks comparing master to 7.9.

/cc @mscdex @mcollina @nodejs/benchmarking

@TimothyGu
Copy link
Member

Cross posting from #12655 (comment):

It's very probable that the regression you are seeing comes from #11930, but IMO it is more of a problem of unrealistic benchmark (see #11930 (comment) for my reasoning).

@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js. labels Apr 25, 2017
@mcollina
Copy link
Member

mcollina commented May 9, 2017

I think the problem for this regression is https://bugs.chromium.org/p/v8/issues/detail?id=6376. Specifically https://github.com/nodejs/node/blob/master/lib/events.js#L378, as the regression is present in the remove benchmarks.

@bmeurer
Copy link
Member

bmeurer commented May 9, 2017

I'm working on the Array.prototype.shift fast-path in TurboFan.

kisg pushed a commit to paul99/v8mips that referenced this issue May 10, 2017
For small arrays, it's way faster to just move the elements instead of
doing the fairly complex and heavy-weight left-trimming. Crankshaft has
had this optimization for small arrays already; this CL more or less
ports this functionality to TurboFan, which yields a 4x speed-up when
using shift on small arrays (with up to 16 elements).

This should recover some of the regressions reported in the Node.js issues

  nodejs/node#12657

and discovered for the syncthrough module using

  https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

as benchmark.

[email protected]
BUG=v8:6376

Review-Url: https://codereview.chromium.org/2874453002
Cr-Commit-Position: refs/heads/master@{#45216}
ofrobots added a commit to ofrobots/node that referenced this issue May 23, 2017
Original commit message:
  [turbofan] Boost performance of Array.prototype.shift by 4x.

  For small arrays, it's way faster to just move the elements instead of
  doing the fairly complex and heavy-weight left-trimming. Crankshaft has
  had this optimization for small arrays already; this CL more or less
  ports this functionality to TurboFan, which yields a 4x speed-up when
  using shift on small arrays (with up to 16 elements).

  This should recover some of the regressions reported in the Node.js issues

    nodejs#12657

  and discovered for the syncthrough module using

    https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

  as benchmark.

  [email protected]
  BUG=v8:6376

  Review-Url: https://codereview.chromium.org/2874453002
  Cr-Commit-Position: refs/heads/master@{nodejs#45216}

PR-URL: nodejs#13162
Reviewed-By: fhinkel - Franziska Hinkelmann <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue May 25, 2017
Original commit message:
  [turbofan] Boost performance of Array.prototype.shift by 4x.

  For small arrays, it's way faster to just move the elements instead of
  doing the fairly complex and heavy-weight left-trimming. Crankshaft has
  had this optimization for small arrays already; this CL more or less
  ports this functionality to TurboFan, which yields a 4x speed-up when
  using shift on small arrays (with up to 16 elements).

  This should recover some of the regressions reported in the Node.js issues

    #12657

  and discovered for the syncthrough module using

    https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

  as benchmark.

  [email protected]
  BUG=v8:6376

  Review-Url: https://codereview.chromium.org/2874453002
  Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13162
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue May 25, 2017
Original commit message:
  [turbofan] Boost performance of Array.prototype.shift by 4x.

  For small arrays, it's way faster to just move the elements instead of
  doing the fairly complex and heavy-weight left-trimming. Crankshaft has
  had this optimization for small arrays already; this CL more or less
  ports this functionality to TurboFan, which yields a 4x speed-up when
  using shift on small arrays (with up to 16 elements).

  This should recover some of the regressions reported in the Node.js issues

    #12657

  and discovered for the syncthrough module using

    https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

  as benchmark.

  [email protected]
  BUG=v8:6376

  Review-Url: https://codereview.chromium.org/2874453002
  Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13162
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this issue May 28, 2017
Original commit message:
  [turbofan] Boost performance of Array.prototype.shift by 4x.

  For small arrays, it's way faster to just move the elements instead of
  doing the fairly complex and heavy-weight left-trimming. Crankshaft has
  had this optimization for small arrays already; this CL more or less
  ports this functionality to TurboFan, which yields a 4x speed-up when
  using shift on small arrays (with up to 16 elements).

  This should recover some of the regressions reported in the Node.js issues

    #12657

  and discovered for the syncthrough module using

    https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

  as benchmark.

  [email protected]
  BUG=v8:6376

  Review-Url: https://codereview.chromium.org/2874453002
  Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13162
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit to targos/node that referenced this issue Jun 6, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      nodejs#12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{nodejs#45216}
targos added a commit to targos/node that referenced this issue Jun 7, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      nodejs#12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{nodejs#45216}

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jun 17, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jun 21, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jun 24, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
rvagg pushed a commit that referenced this issue Jun 29, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jul 11, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jul 18, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos added a commit to targos/node that referenced this issue Jul 21, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      nodejs#12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{nodejs#45216}

PR-URL: nodejs#13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
addaleax pushed a commit that referenced this issue Jul 24, 2017
Original commit message:

    [turbofan] Boost performance of Array.prototype.shift by 4x.

    For small arrays, it's way faster to just move the elements instead of
    doing the fairly complex and heavy-weight left-trimming. Crankshaft has
    had this optimization for small arrays already; this CL more or less
    ports this functionality to TurboFan, which yields a 4x speed-up when
    using shift on small arrays (with up to 16 elements).

    This should recover some of the regressions reported in the Node.js issues

      #12657

    and discovered for the syncthrough module using

      https://github.com/mcollina/syncthrough/blob/master/benchmarks/basic.js

    as benchmark.

    [email protected]
    BUG=v8:6376

    Review-Url: https://codereview.chromium.org/2874453002
    Cr-Commit-Position: refs/heads/master@{#45216}

PR-URL: #13263
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@Trott
Copy link
Member

Trott commented Aug 13, 2017

Comparing 8.3.0 with 7.9.0:

$ cat compare.csv | Rscript benchmark/compare.R 
                                                     improvement confidence      p.value
 events/ee-add-remove.js n=250000                       -30.57 %        *** 3.570833e-60
 events/ee-emit-multi-args.js n=2000000                 -13.31 %        *** 1.491800e-38
 events/ee-emit.js n=2000000                            -14.58 %        *** 1.977752e-27
 events/ee-listener-count-on-prototype.js n=50000000     12.87 %        *** 4.639347e-34
 events/ee-listeners-many.js n=5000000                   34.51 %        *** 1.745743e-50
 events/ee-listeners.js n=5000000                        19.73 %        *** 6.707594e-34
 events/ee-once.js n=20000000                           -40.54 %        *** 5.223855e-52
$

@bmeurer
Copy link
Member

bmeurer commented Aug 13, 2017

Those numbers were taken using the 8.3.0 versions of those benchmarks?

@Trott
Copy link
Member

Trott commented Aug 13, 2017

Those numbers were taken using the 8.3.0 versions of those benchmarks?

I used the version in the master branch.

@Trott
Copy link
Member

Trott commented Aug 13, 2017

Also: I ran it on macOS 10.12.6.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 13, 2017

I've tested on Windows 7 x 64, vs V8 6.0 / 6.1 / 6.2 and things seem getting better:

V8 5.5.372.43 (Node.js 7.10.1) vs V8 6.0.286.52 (Node.js 8.3.0)
                                                      improvement confidence      p.value
 events\\ee-add-remove.js n=250000                       -42.77 %        *** 4.141429e-90
 events\\ee-emit-multi-args.js n=2000000                  -7.56 %        *** 3.765808e-27
 events\\ee-emit.js n=2000000                             -9.59 %        *** 5.277457e-27
 events\\ee-listener-count-on-prototype.js n=50000000     12.72 %        *** 9.573892e-38
 events\\ee-listeners-many.js n=5000000                   21.15 %        *** 4.807840e-43
 events\\ee-listeners.js n=5000000                         6.91 %        *** 1.381210e-06
 events\\ee-once.js n=20000000                           -45.65 %        *** 1.205400e-57


V8 5.5.372.43 (Node.js 7.10.1) vs V8 6.1.556 (Node.js 9.0.0 2017-07-20 V8-canary)
                                                      improvement confidence      p.value
 events\\ee-add-remove.js n=250000                       -12.04 %        *** 1.846156e-35
 events\\ee-emit-multi-args.js n=2000000                  10.96 %        *** 1.395131e-29
 events\\ee-emit.js n=2000000                              2.63 %        *** 5.275229e-05
 events\\ee-listener-count-on-prototype.js n=50000000      8.65 %        *** 3.501371e-33
 events\\ee-listeners-many.js n=5000000                   18.98 %        *** 3.670456e-37
 events\\ee-listeners.js n=5000000                         5.45 %        *** 8.625583e-10
 events\\ee-once.js n=20000000                           -31.56 %        *** 1.767708e-43


V8 5.5.372.43 (Node.js 7.10.1) vs V8 6.2.217 (Node.js 9.0.0 2017-08-12 V8-canary)
                                                      improvement confidence      p.value
 events\\ee-add-remove.js n=250000                        -9.71 %        *** 7.300658e-24
 events\\ee-emit-multi-args.js n=2000000                  21.74 %        *** 5.781329e-30
 events\\ee-emit.js n=2000000                             10.67 %        *** 3.856524e-20
 events\\ee-listener-count-on-prototype.js n=50000000      6.88 %        *** 2.059200e-11
 events\\ee-listeners-many.js n=5000000                   14.08 %        *** 3.452212e-45
 events\\ee-listeners.js n=5000000                        -1.82 %         ** 1.769813e-03
 events\\ee-once.js n=20000000                           -26.43 %        *** 5.119348e-73

@apapirovski
Copy link
Member

There doesn't seem to be much actionable here. The benchmarks should probably be updated to be more comprehensive but that's an issue across the board. The performance as per above is now nearly at par or better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

No branches or pull requests

8 participants