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: remove input param manipulation #41741

Closed
wants to merge 1 commit into from

Conversation

MrJithil
Copy link
Member

Worked on a TODO which demands to avoid the manipulation of input param

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 29, 2022
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thanks 🙏

@MrJithil
Copy link
Member Author

/test

@benjamingr
Copy link
Member

@MrJithil there are two kinds of tests in Node:

  • There are tests that run on every PR automatically, you can see them under "checks".
  • There is a more extensive CI process which only collaborators can trigger due to risk potential.

I've gone ahead and added a label to this PR to automatically trigger CI :)

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@MrJithil
Copy link
Member Author

Thanks @benjamingr

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2022
@nodejs-github-bot

This comment has been minimized.

@Mesteery
Copy link
Contributor

Mesteery commented Jan 29, 2022

request-ci will not launch or test this benchmark, or I am wrong? @benjamingr

@benjamingr
Copy link
Member

benjamingr commented Jan 29, 2022

@targos
Copy link
Member

targos commented Jan 29, 2022

There are some benchmark tests in CI. I don't know how the coverage for them is though.

Edit: Here https://github.com/nodejs/node/blob/master/test/benchmark/test-benchmark-assert.js

@benjamingr
Copy link
Member

@Mesteery apparently the CI does test benchmarks, I didn't know that and @Trott showed me

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jan 30, 2022

There are some benchmark tests in CI. I don't know how the coverage for them is though.

The coverage is minimal. Each benchmark file runs exactly one condition and that's it. The purpose is to make sure there aren't benchmarks that are completely broken and can't run at all. That used to happen.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 1, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MrJithil
Copy link
Member Author

MrJithil commented Feb 3, 2022

/request-ci

@nodejs-github-bot
Copy link
Collaborator

@Mesteery Mesteery added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41741
✔  Done loading data for nodejs/node/pull/41741
----------------------------------- PR info ------------------------------------
Title      benchmark: remove input param manipulation (#41741)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     MrJithil:todo-task-4 -> nodejs:master
Labels     assert, test, benchmark, needs-ci
Commits    1
 - benchmark: avoid input param manipulation
Committers 1
 - MrJithil 
PR-URL: https://github.com/nodejs/node/pull/41741
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Mestery 
Reviewed-By: Luigi Pinca 
Reviewed-By: Colin Ihrig 
Reviewed-By: Mary Marchini 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41741
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Mestery 
Reviewed-By: Luigi Pinca 
Reviewed-By: Colin Ihrig 
Reviewed-By: Mary Marchini 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 29 Jan 2022 05:53:10 GMT
   ✔  Approvals: 6
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41741#pullrequestreview-866902469
   ✔  - Mohammed Keyvanzadeh (@VoltrexMaster): https://github.com/nodejs/node/pull/41741#pullrequestreview-866928878
   ✔  - Mestery (@Mesteery): https://github.com/nodejs/node/pull/41741#pullrequestreview-866932788
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41741#pullrequestreview-867009867
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/41741#pullrequestreview-867200235
   ✔  - Mary Marchini (@mmarchini) (TSC): https://github.com/nodejs/node/pull/41741#pullrequestreview-867225186
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2022-02-03T17:32:44Z: https://ci.nodejs.org/job/node-test-pull-request/42335/
   ℹ  Last Benchmark CI on 2022-01-31T10:28:30Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1091/
- Querying data for job/node-test-pull-request/42335/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1791088414

@Mesteery
Copy link
Contributor

Mesteery commented Feb 3, 2022

I don't understand why one of the jobs is still marked as running on GitHub, while the same job is successful on Jenkins.

@MrJithil
Copy link
Member Author

MrJithil commented Feb 3, 2022

Help wanted on this

jasnell pushed a commit that referenced this pull request Feb 5, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 5, 2022

Landed in 217acb9

@jasnell jasnell closed this Feb 5, 2022
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
PR-URL: nodejs#41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
PR-URL: nodejs#41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 8, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41741
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2023

I just noticed this and it seems like my comment was misunderstood :).

Any variable coming from the tests definition should be taken one by one instead of being changed inside of the method as the definition is printed as it is. If the method uses something else, the definition does not fit anymore.

n should be the number of runs. The reason for this was that it was not possible to define the defintion to keep the benchmark below a runtime threshold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.