GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: avoid copying strings in FSPermission::Apply by tniessen · Pull Request #50662 · nodejs/node

Conversation

@tniessen

@tniessen tniessen commented Nov 10, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@tniessen 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
tniessen requested review from RafaelGSS and anonrig November 10, 2023 20:22
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/security-wg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 10, 2023
anonrig
anonrig approved these changes Nov 10, 2023
View reviewed changes
@anonrig anonrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 10, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 10, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Nov 11, 2023
CI Reliability 2023-11-11 nodejs/reliability#714
Open
27 tasks
RafaelGSS
RafaelGSS approved these changes Nov 11, 2023
View reviewed changes
marco-ippolito
marco-ippolito approved these changes Nov 11, 2023
View reviewed changes
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Nov 11, 2023
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Nov 12, 2023
CI Reliability 2023-11-12 nodejs/reliability#715
Open
18 tasks
lpinca
lpinca approved these changes Nov 12, 2023
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Nov 13, 2023
CI Reliability 2023-11-13 nodejs/reliability#716
Open
24 tasks
@tniessen tniessen added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 13, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 13, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 13, 2023

Copy link
Copy Markdown
Collaborator
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen tniessen added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 13, 2023
@tniessen

tniessen commented Nov 13, 2023

Copy link
Copy Markdown
Member Author

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:

screenshot of test-asan

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Nov 14, 2023
CI Reliability 2023-11-14 nodejs/reliability#717
Open
25 tasks
@tniessen

tniessen commented Nov 17, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen
src: avoid copying strings in FSPermission::Apply
2ee82d5 
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
@tniessen
tniessen force-pushed the src-fs-perm-string-alloc branch from 0faf06f to 2ee82d5 Compare November 17, 2023 13:41
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Nov 19, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Nov 19, 2023

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Nov 19, 2023
View reviewed changes
@jasnell jasnell added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 19, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 19, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 48f32d7 into nodejs:main Nov 19, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 19, 2023

Copy link
Copy Markdown
Collaborator

Landed in 48f32d7
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request Nov 23, 2023
@tniessen @targos
src: avoid copying strings in FSPermission::Apply
e2862ab 
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 <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@tniessen Lei Shi
src: avoid copying strings in FSPermission::Apply
c801888 
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 <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
v21.3.0 proposal #50954
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
@tniessen @RafaelGSS
src: avoid copying strings in FSPermission::Apply
0fb35b6 
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 <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@tniessen @UlisesGascon
src: avoid copying strings in FSPermission::Apply
0e7acf9 
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 <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@tniessen @UlisesGascon
src: avoid copying strings in FSPermission::Apply
d08eb38 
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 <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
@anonrig anonrig anonrig approved these changes
@RafaelGSS RafaelGSS RafaelGSS approved these changes
@marco-ippolito marco-ippolito marco-ippolito approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@tniessen @nodejs-github-bot @jasnell @lpinca @anonrig @RafaelGSS @marco-ippolito
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.