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.

Sign up for GitHub

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

Jump to bottom

src: disable fast methods for buffer.write #54565

Merged
merged 1 commit into from
Aug 29, 2024
Merged

src: disable fast methods for buffer.write #54565

merged 1 commit into from
Aug 29, 2024

Conversation

targos
Copy link
Member

@targos targos commented Aug 26, 2024

It should resolve the regressions while we work on fixing them.

Refs: #54521

@targos
src: disable fast methods for buffer.write
a7a10a7 
It should resolve the regressions while we work on fixing them.

Refs: nodejs#54521
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 26, 2024
@targos targos mentioned this pull request Aug 26, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Aug 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (1399d4e) to head (a7a10a7).
Report is 323 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54565      +/-   ##
==========================================
- Coverage   87.33%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182626   182620       -6     
  Branches    35041    35040       -1     
==========================================
- Hits       159503   159489      -14     
- Misses      16394    16404      +10     
+ Partials     6729     6727       -2
Files with missing lines Coverage Δ
src/node_buffer.cc 69.94% <100.00%> (-0.71%) ⬇️

... and 23 files with indirect coverage changes

@targos
Copy link
Member Author

targos commented Aug 26, 2024

If this is accepted, it should be fast-tracked for inclusion in #54560

@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

Fast-track has been requested by @targos. Please 👍 to approve.

@benjamingr
Copy link
Member

@bricss please do not approve fast-track requests :] (or you know, start making PRs and join as a collaborator that also works :))

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev mentioned this pull request Aug 26, 2024
Closed
@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member

ronag commented Aug 27, 2024

@targos can you fix commit msg (PR-URL)?

@RedYetiDev RedYetiDev mentioned this pull request Aug 27, 2024
Closed
jasnell
jasnell reviewed Aug 27, 2024
View reviewed changes
src/node_buffer.cc
"utf8WriteStatic",
SlowWriteString<UTF8>,
&fast_write_string);
SetMethod(context, target, "asciiWriteStatic", SlowWriteString<ASCII>);
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking nit: perhaps consider adding a TODO comment?

@targos
Copy link
Member Author

targos commented Aug 27, 2024

@ronag I don't see a problem in the commit message.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member

ronag commented Aug 28, 2024 •
edited
Loading

@ronag I don't see a problem in the commit message.

@targos

Commit message must match a given regex pattern: PR-URL: https://github\.com/(nodejs|nodejs-private)/(node|node-private)/pull/\d+

@targos
Copy link
Member Author

targos commented Aug 28, 2024

This message appears (to collaborators) on all the pull requests of the project. Unfortunately there is no way to customize it but the goal is to prevent merging by accident using the GitHub UI.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7616855 into nodejs:main Aug 29, 2024
74 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7616855

@targos targos deleted the buffer-fast-write-disable branch August 29, 2024 07:24
@RedYetiDev RedYetiDev mentioned this pull request Aug 29, 2024
Closed
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@targos @RafaelGSS
src: disable fast methods for buffer.write
50dec7c 
It should resolve the regressions while we work on fixing them.

Refs: #54521
PR-URL: #54565
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@targos @RafaelGSS
src: disable fast methods for buffer.write
7c3599b 
It should resolve the regressions while we work on fixing them.

Refs: #54521
PR-URL: #54565
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@targos @RafaelGSS
src: disable fast methods for buffer.write
e071651 
It should resolve the regressions while we work on fixing them.

Refs: #54521
PR-URL: #54565
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@emilazy emilazy mentioned this pull request Sep 3, 2024
Merged
13 tasks
trentm added a commit to trentm/opentelemetry-js that referenced this pull request Sep 4, 2024
@trentm
chore: unpin unit-test from 22.6
3d69d6b 
Now that Node.js 22.8 is available with a workaround
(nodejs/node#54565) for the bug in 22.7 that
caused 'RangeError: "length" is outside of buffer bounds' errors in
exporter tests (see open-telemetry#4953),
we can unpin the Node.js v22 used for unit tests.
This undoes the pinning from open-telemetry#4957.
@trentm trentm mentioned this pull request Sep 4, 2024
Merged
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@targos @louwers
src: disable fast methods for buffer.write
c09e696 
It should resolve the regressions while we work on fixing them.

Refs: nodejs#54521
PR-URL: nodejs#54565
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[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

@ronag ronag ronag approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@targos @benjamingr @nodejs-github-bot @ronag @jasnell @addaleax @lpinca @anonrig