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.

Sign up for GitHub

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

Jump to bottom

timers "depth" benchmark usefulness #9493

Closed
Trott opened this issue Nov 6, 2016 · 8 comments
Closed

timers "depth" benchmark usefulness #9493

Trott opened this issue Nov 6, 2016 · 8 comments
Assignees
@Fishrock123
Labels
benchmark Issues and PRs related to the benchmark subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@Trott
Copy link
Member

Trott commented Nov 6, 2016 •
edited
Loading

  • Version: v8.0.0-pre
  • Platform: all
  • Subsystem: benchmark, timers

The depth benchmark for timers sets a timer that sets a timer that sets a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop:

  • This benchmark takes a very long time to run compared to the breadth test that is already in the file.
  • This may be more of an event loop benchmark than a timer benchmark.

I wonder if it makes sense to do any of the following or something similar:

  • Reduce the number of iterations for the depth test as it's really just running the iterations in sequence, not in parallel. And even on an infinitely fast machine, it would take over 8 minutes to run because each tick of the event loop would have to wait 1ms before firing the timer.

  • Move and/or rename the depth benchmark as it is unlikely to be something significantly impacted by changes in the Node.js timers code.

I know I can send command line arguments to skip the depth test or change the value of N. I just suspect that the default behavior right now isn't ideal. Every time I touch timers code and run a benchmark, this is an annoyance.
@Trott Trott added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. benchmark Issues and PRs related to the benchmark subsystem. labels Nov 6, 2016
@Trott
Copy link
Member Author

Trott commented Nov 6, 2016

@mscdex @misterdjules @Fishrock123 @AndreasMadsen

Oh, I'll also note that it was moved from misc to timers earlier this year without apparent significant disruption, so presumably another move or split would not be disruptive either.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Nov 6, 2016 •
edited
Loading

We could probably remove it, a test assertion asserting that timers are indeed pooled in this case mentioning benchmarks should be good enough combined with other benchmarks.

@Fishrock123
Copy link
Contributor

Hmmm, at a second look I definitely think there are not enough benchmarks to replace it yet.

@Fishrock123 Fishrock123 self-assigned this Nov 6, 2016
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Nov 6, 2016 •
edited
Loading

There are definitely benchmarks that uses many more iterations than what is required. I guess this is one of them.

I'm don't like moving the benchmark to a diffrent category, I think timer benchmarks should be in the timer category. @Fishrock123 suggestion sounds reasonable!

If you are interested in reducing the number of iterations, a rough estimate on the appropriate number of iterations could be found by tuning the coefficient of variation std(x)/mean(x) (use the unbiased estimate). See #8139 (comment) for the practical meaning of this.

@mscdex
Copy link
Contributor

mscdex commented Nov 6, 2016

I think in general there are lots of cases like this in many of the different benchmarks where some configurations take longer than others but the same iteration count is used for all of them. I don't know of a good way to solve this, since implicitly altering the iteration count for certain configurations could be seen as unexpected (even if the new iteration count is reported in the output).

@Trott
Copy link
Member Author

Trott commented Nov 6, 2016

Maybe it makes sense to split this benchmark into two files and but leave them both in the timers directory? This way they can have separate N values but both will still be categorzied as timer benchmarks.

@Trott Trott mentioned this issue Nov 7, 2016
Closed
3 tasks
@Trott
Copy link
Member Author

Trott commented Nov 7, 2016

#9497

Trott added a commit to Trott/io.js that referenced this issue Nov 8, 2016
@Trott
benchmark: split timers benchmark and refactor
7d60498 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: nodejs#9493
Trott added a commit to Trott/io.js that referenced this issue Nov 10, 2016
@Trott
benchmark: split timers benchmark and refactor
ef6a5cf 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: nodejs#9493
PR-URL: nodejs#9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit that referenced this issue Nov 22, 2016
@Trott @addaleax
benchmark: split timers benchmark and refactor
819a38d 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
@Trott @MylesBorins
benchmark: split timers benchmark and refactor
9c08a03 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
@Trott @MylesBorins
benchmark: split timers benchmark and refactor
98b2eae 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
@Trott @MylesBorins
benchmark: split timers benchmark and refactor
b1075f6 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
@Trott @MylesBorins
benchmark: split timers benchmark and refactor
e8e9cca 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
@Trott @MylesBorins
benchmark: split timers benchmark and refactor
8f58d25 
The depth benchmark for timers sets a timer that sets a timer that sets
a timer that... 500K of them.

Since each timer has to wait for the next tick of the event loop this
benchmark takes a very long time to run compared to the breadth
test that is already in the file. This may be more of an event loop
benchmark than a timer benchmark.

Reduce the number of iterations for the depth test as it's really just
running the iterations in sequence, not in parallel. And even on an
infinitely fast machine, it would take over 8 minutes to run because
each tick of the event loop would have to wait 1ms before firing the
timer.

Split the depth and breadth benchmarks so that their `N` values can be
set independently.

Do some minor refactoring to the benchmarks (but no ES6 additions so
that the benchmarks can still be run with old versions of Node.js).

Refs: #9493
PR-URL: #9497
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123 Fishrock123 mentioned this issue Jan 20, 2017
Merged
3 tasks
Fishrock123 added a commit to Fishrock123/node that referenced this issue Jan 27, 2017
@Fishrock123
benchmark: add more thorough timers benchmarks
c74e6fe 
Refs: nodejs#9493
PR-URL: nodejs#10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jan 30, 2017 •
edited
Loading

This should be fixed by c74e6fe (#10925)

evanlucas pushed a commit that referenced this issue Jan 31, 2017
@Fishrock123 @evanlucas
benchmark: add more thorough timers benchmarks
062c851 
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
jasnell pushed a commit that referenced this issue Mar 8, 2017
@Fishrock123 @jasnell
benchmark: add more thorough timers benchmarks
dc207cd 
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
jasnell pushed a commit that referenced this issue Mar 8, 2017
@Fishrock123 @jasnell
benchmark: add more thorough timers benchmarks
c004d43 
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
@Fishrock123 @MylesBorins
benchmark: add more thorough timers benchmarks
4d6fa8d 
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
@Fishrock123 @MylesBorins
benchmark: add more thorough timers benchmarks
14e57c1 
Refs: #9493
PR-URL: #10925
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Fishrock123 Fishrock123

Labels
benchmark Issues and PRs related to the benchmark subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mscdex @AndreasMadsen @Trott @Fishrock123