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

benchmark: always throw the same Error instance #34523

Closed

Conversation

addaleax
Copy link
Member

Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: #34512 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: nodejs#34512 (comment)
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels Jul 26, 2020
@jasnell
Copy link
Member

jasnell commented Jul 27, 2020

Hmm, I'm torn on this. While I get the reason for the change, it makes the benchmark even more unrealistic compared to real world user code. Our microbenchmarks are already overly optimized as it is. Not a blocking concern, however.

@addaleax
Copy link
Member Author

@jasnell I mean, that really depends on what we’re measuring here, right? The point is to measure the performance effect of async_hooks on Promises. Let’s leave that out which isn’t relevant to it.

(For example, the other async_hooks benchmarks are very diluted because they’re full-blown HTTP servers – that might be closer to a real-world example, but it makes it harder to measure the perf impact of async_hooks themselves. For profiling, they are basically useless and I always needed to go to the MessagePort benchmarks instead.)

@jasnell
Copy link
Member

jasnell commented Jul 27, 2020

Yeah like I said, I'm torn on it and don't consider it blocking. I just think we way over index on microbenchmarks in general.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 27, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/32518/

puzpuzpuz pushed a commit that referenced this pull request Jul 29, 2020
Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: #34512 (comment)

PR-URL: #34523
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@puzpuzpuz
Copy link
Member

Landed in b14ce72

@puzpuzpuz puzpuzpuz closed this Jul 29, 2020
codebytere pushed a commit that referenced this pull request Aug 5, 2020
Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: #34512 (comment)

PR-URL: #34523
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@codebytere codebytere mentioned this pull request Aug 10, 2020
addaleax added a commit that referenced this pull request Sep 22, 2020
Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: #34512 (comment)

PR-URL: #34523
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
addaleax added a commit that referenced this pull request Sep 22, 2020
Stack trace capturing currently accounts for 40 % of the benchmark
running time. Always throwing the same exception object removes
that overhead and lets the benchmark be more focused on what it is
supposed to measure.

Refs: #34512 (comment)

PR-URL: #34523
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants