Skip to content

benchmark: always throw the same Error instance - #34523

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:benchmark-async-hooks-promises
Closed

benchmark: always throw the same Error instance#34523
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:benchmark-async-hooks-promises

Conversation

@addaleax

Copy link
Copy Markdown
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 Node.js benchmarks and benchmarking infrastructure. labels Jul 26, 2020
@jasnell

jasnell commented Jul 27, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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

jasnell commented Jul 27, 2020

Copy link
Copy Markdown
Member

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 with CI started, the required approvals, and no outstanding review comments. label Jul 27, 2020
@nodejs-github-bot

Copy link
Copy Markdown
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
Copy Markdown
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 with CI started, the required approvals, and no outstanding review comments. benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants