Skip to content

lib: improve performance of validateStringArray and validateBooleanArray - #49756

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Uzlopak:improve-perf-array-validators
Oct 22, 2023
Merged

lib: improve performance of validateStringArray and validateBooleanArray#49756
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Uzlopak:improve-perf-array-validators

Conversation

@Uzlopak

@UzlopakUzlopak commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

instead of calling the validateString/validateBoolean function, where we are forced to create strings, which we potentially dont need, we create the string only when needed.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1423

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 21, 2023
Comment threadlib/internal/validators.js Outdated
@anonrig

Copy link
Copy Markdown
Member

Since you are creating a new benchmark suite, you need to also create the appropriate test for it. Similar to https://github.com/nodejs/node/blob/main/test/benchmark/test-bechmark-readline.js

@anonriganonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 22, 2023
@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

@anonrig
@mscdex
PTAL

@anonriganonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-benchmark-ci PR that need a benchmark CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 22, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

Can this please be merged?

I think I improved the performance of parseFileMode and I would like to use the benchmark in this PR.

functionparseFileMode(value,name,def){value??=def;if(typeofvalue==='string'){if(value===''){thrownewERR_INVALID_ARG_VALUE(name,value,modeDesc);}letresult=0;for(leti=0;i<value.length;++i){result<<=3;switch(value[i]){case'7':
result+=7;break;case'6':
result+=6;break;case'5':
result+=5;break;case'4':
result+=4;break;case'3':
result+=3;break;case'2':
result+=2;break;case'1':
result+=1;break;case'0':
break;default:
thrownewERR_INVALID_ARG_VALUE(name,value,modeDesc);}}returnresult;}validateUint32(value,name);returnvalue;}

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@targos

Copy link
Copy Markdown
Member

It needs a benchmark run

@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

@anonrig
@aduh95

can one of you please run the benchmarks?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

@aduh95

The failing tests have to be unrelated. Can you retrigger the tests, please? or should i rebase, because the fixes for the failing tests is in master already?

@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

@aduh95
@anonrig
Whats up? Can we merge?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

@aduh95
@anonrig

Tests finally pass. Lets merge:)?

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/49756
✔ Done loading data for nodejs/node/pull/49756
----------------------------------- PR info ------------------------------------
Title lib: improve performance of validateStringArray and validateBooleanArray (#49756)
Author Aras Abbasi (@Uzlopak)
Branch Uzlopak:improve-perf-array-validators -> nodejs:main
Labels performance, author ready, needs-ci, needs-benchmark-ci
Commits 1
- lib: improve performance of validateStringArray and validateBooleanArray
Committers 1
- uzlopak PR-URL: https://github.com/nodejs/node/pull/49756
Reviewed-By: Yagiz Nizipli Reviewed-By: Trivikram Kamat ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49756
Reviewed-By: Yagiz Nizipli Reviewed-By: Trivikram Kamat --------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - lib: improve performance of validateStringArray and validateBooleanArray
ℹ This PR was created on Thu, 21 Sep 2023 21:54:17 GMT
✔ Approvals: 2
✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49756#pullrequestreview-1641142923
✔ - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/49756#pullrequestreview-1641181220
✔ Last GitHub CI successful
ℹ Last Benchmark CI on 2023-09-27T13:36:16Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1423
ℹ Last Full PR CI on 2023-10-22T19:56:46Z: https://ci.nodejs.org/job/node-test-pull-request/55130/
- Querying data for job/node-test-pull-request/55130/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6606164183

@nodejs-github-botnodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 22, 2023
@anonriganonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 22, 2023
@Uzlopak

Copy link
Copy Markdown
ContributorAuthor

image

@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2023
@nodejs-github-bot
nodejs-github-bot merged commit a58ffad into nodejs:mainOct 22, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in a58ffad

@Uzlopak
Uzlopak deleted the improve-perf-array-validators branch October 22, 2023 21:16
targos pushed a commit that referenced this pull request Oct 23, 2023
PR-URL: #49756
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targostargos mentioned this pull request Oct 23, 2023
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49756
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targostargos mentioned this pull request Nov 12, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Uzlopak@anonrig@nodejs-github-bot@targos@aduh95@mscdex@trivikr