src: avoid copying strings in FSPermission::Apply #50662

tniessen · 2023-11-10T20:22:59Z

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a791, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings.

Refs: #48491
Refs: #49047

nodejs-github-bot · 2023-11-10T20:23:05Z

Review requested:

@nodejs/security-wg

nodejs-github-bot · 2023-11-11T10:59:02Z

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

nodejs-github-bot · 2023-11-13T12:30:53Z

Commit Queue failed

- Loading data for nodejs/node/pull/50662
✔  Done loading data for nodejs/node/pull/50662
----------------------------------- PR info ------------------------------------
Title      src: avoid copying strings in FSPermission::Apply (#50662)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     tniessen:src-fs-perm-string-alloc -> nodejs:main
Labels     c++, experimental, author ready, needs-ci, permission
Commits    1
 - src: avoid copying strings in FSPermission::Apply
Committers 1
 - Tobias Nießen 
PR-URL: https://github.com/nodejs/node/pull/50662
Refs: https://github.com/nodejs/node/pull/48491
Refs: https://github.com/nodejs/node/pull/49047
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: Marco Ippolito 
Reviewed-By: James M Snell 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50662
Refs: https://github.com/nodejs/node/pull/48491
Refs: https://github.com/nodejs/node/pull/49047
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Rafael Gonzaga 
Reviewed-By: Marco Ippolito 
Reviewed-By: James M Snell 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 10 Nov 2023 20:22:59 GMT
   ✔  Approvals: 5
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50662#pullrequestreview-1725576719
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/50662#pullrequestreview-1725854133
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/50662#pullrequestreview-1726036425
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50662#pullrequestreview-1726071194
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/50662#pullrequestreview-1726185407
   ✘  GitHub CI is still running
   ℹ  Last Full PR CI on 2023-11-11T10:59:02Z: https://ci.nodejs.org/job/node-test-pull-request/55552/
- Querying data for job/node-test-pull-request/55552/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6849962960

tniessen · 2023-11-13T13:38:07Z

It seems that the issue is test-asan, which failed due to a flake and was rescheduled three days ago but still shows as queued:

I don't see any option to cancel it or to start another run.

tniessen · 2023-11-17T13:40:07Z

I honestly have no idea how to get this unstuck except by force pushing, which will require a new CI run...

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a791, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: nodejs#48491 Refs: nodejs#49047

nodejs-github-bot · 2023-11-19T14:11:40Z

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

tniessen · 2023-11-19T16:04:44Z

Please re-approve due to the reason described above and add the commit-queue label.

nodejs-github-bot · 2023-11-19T20:21:37Z

Landed in 48f32d7

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a791, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: #48491 Refs: #49047 PR-URL: #50662 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a791, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: nodejs#48491 Refs: nodejs#49047 PR-URL: nodejs#50662 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The use of string_view and subsequent copying to a string was supposed to be a minor optimization in 640a791, however, since 413c16e, no string splitting occurs anymore. Therefore, we can simply pass around some references instead of using string_view or copying strings. Refs: #48491 Refs: #49047 PR-URL: #50662 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

tniessen added c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. permission Issues and PRs related to the Permission Model labels Nov 10, 2023

tniessen requested review from anonrig and RafaelGSS November 10, 2023 20:22

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 10, 2023

anonrig approved these changes Nov 10, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 10, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2023

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Nov 11, 2023

CI Reliability 2023-11-11 nodejs/reliability#714

Open

27 tasks

RafaelGSS approved these changes Nov 11, 2023

View reviewed changes

marco-ippolito approved these changes Nov 11, 2023

View reviewed changes

jasnell approved these changes Nov 11, 2023

View reviewed changes

github-actions bot mentioned this pull request Nov 12, 2023

CI Reliability 2023-11-12 nodejs/reliability#715

Open

18 tasks

lpinca approved these changes Nov 12, 2023

View reviewed changes

github-actions bot mentioned this pull request Nov 13, 2023

CI Reliability 2023-11-13 nodejs/reliability#716

Open

24 tasks

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2023

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 Nov 13, 2023

tniessen 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. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 13, 2023

github-actions bot mentioned this pull request Nov 14, 2023

CI Reliability 2023-11-14 nodejs/reliability#717

Open

25 tasks

tniessen force-pushed the src-fs-perm-string-alloc branch from 0faf06f to 2ee82d5 Compare November 17, 2023 13:41

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2023

This comment was marked as outdated.

Sign in to view

jasnell approved these changes Nov 19, 2023

View reviewed changes

jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2023

nodejs-github-bot merged commit 48f32d7 into nodejs:main Nov 19, 2023
53 checks passed

RafaelGSS mentioned this pull request Nov 28, 2023

v21.3.0 proposal #50954

Merged

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: avoid copying strings in FSPermission::Apply #50662

src: avoid copying strings in FSPermission::Apply #50662

tniessen commented Nov 10, 2023

nodejs-github-bot commented Nov 10, 2023

This comment was marked as outdated.

nodejs-github-bot commented Nov 11, 2023 •

edited by jasnell

nodejs-github-bot commented Nov 13, 2023

tniessen commented Nov 13, 2023

tniessen commented Nov 17, 2023

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Nov 19, 2023

tniessen commented Nov 19, 2023

nodejs-github-bot commented Nov 19, 2023

src: avoid copying strings in FSPermission::Apply #50662

src: avoid copying strings in FSPermission::Apply #50662

Conversation

tniessen commented Nov 10, 2023

nodejs-github-bot commented Nov 10, 2023

This comment was marked as outdated.

nodejs-github-bot commented Nov 11, 2023 • edited by jasnell

nodejs-github-bot commented Nov 13, 2023

tniessen commented Nov 13, 2023

tniessen commented Nov 17, 2023

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Nov 19, 2023

tniessen commented Nov 19, 2023

nodejs-github-bot commented Nov 19, 2023

nodejs-github-bot commented Nov 11, 2023 •

edited by jasnell