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

lib: improve performance of validateStringArray and validateBooleanArray #49756

Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 21, 2023

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-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 21, 2023
@anonrig
Copy link
Member

anonrig commented Sep 22, 2023

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

@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 22, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 22, 2023

@anonrig
@mscdex
PTAL

@anonrig anonrig 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 27, 2023

Can this please be merged?

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

function parseFileMode(value, name, def) {
  value ??= def;
  if (typeof value === 'string') {
    if (value === '') {
      throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
    }
    let result = 0;
    for (let i = 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:
          throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
      }
    }
    return result;
  }

  validateUint32(value, name);
  return value;
}

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Sep 27, 2023

It needs a benchmark run

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 27, 2023

@anonrig
@aduh95

can one of you please run the benchmarks?

@nodejs-github-bot
Copy link
Collaborator

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 16, 2023

@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
Contributor Author

Uzlopak commented Oct 21, 2023

@aduh95
@anonrig
Whats up? Can we merge?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 22, 2023

@aduh95
@anonrig

Tests finally pass. Lets merge:)?

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2023
@nodejs-github-bot nodejs-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
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-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 22, 2023
@anonrig anonrig 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
Contributor Author

Uzlopak commented Oct 22, 2023

image

@nodejs-github-bot nodejs-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:main Oct 22, 2023
59 checks passed
@nodejs-github-bot
Copy link
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>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49756
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants