Skip to content

buffer: avoid creating unnecessary environment#58053

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
anonrig:yagiz/avoid-environment
Apr 29, 2025
Merged

buffer: avoid creating unnecessary environment#58053
nodejs-github-bot merged 1 commit intonodejs:mainfrom
anonrig:yagiz/avoid-environment

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Apr 27, 2025 •
edited
Loading

We don't have to deference the args pointer to create an environment variable while having access to isolate using args.GetIsolate()

@anonrig anonrig requested review from jasnell and ronag April 27, 2025 17:22
@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 Apr 27, 2025
@anonrig anonrig force-pushed the yagiz/avoid-environment branch from 523f5a8 to 4df7266 Compare April 27, 2025 17:24
juanarbol
juanarbol approved these changes Apr 27, 2025
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

sgtm

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

nodejs-github-bot commented Apr 27, 2025

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

jasnell
jasnell approved these changes Apr 27, 2025
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

To be fair, the changed code is not creating an Environment unnecessarily, it is simply dereferencing a pointer.

@anonrig
Copy link
Member Author

anonrig commented Apr 27, 2025

To be fair, the changed code is not creating an Environment unnecessarily, it is simply dereferencing a pointer.

Ah true. Thanks. Still seems like an unnecessary operation that we do in almost all places.

@codecov
Copy link

codecov bot commented Apr 27, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (e0cf8ae) to head (4df7266).
Report is 14 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58053      +/-   ##
==========================================
+ Coverage   90.18%   90.20%   +0.01%     
==========================================
  Files         630      630              
  Lines      186393   186393              
  Branches    36595    36594       -1     
==========================================
+ Hits       168103   168140      +37     
+ Misses      11090    11081       -9     
+ Partials     7200     7172      -28
Files with missing lines Coverage Δ
src/node_buffer.cc 67.51% <100.00%> (ø)

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 27, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 27, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 28, 2025

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

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2025
@nodejs-github-bot nodejs-github-bot merged commit adc948a into nodejs:main Apr 29, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 29, 2025

Landed in adc948a

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@anonrig @RafaelGSS
buffer: avoid creating unnecessary environment
c2150e0 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@anonrig @RafaelGSS
buffer: avoid creating unnecessary environment
ca86c93 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
f89b36e 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
1c8e476 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@anonrig @RafaelGSS
buffer: avoid creating unnecessary environment
aa14ff3 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
9ee074e 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
aa35fd8 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
ad2e1a8 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
8ba2823 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
e95a83e 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@github-actions github-actions bot mentioned this pull request May 19, 2025
Merged
aduh95 pushed a commit that referenced this pull request May 19, 2025
@anonrig @aduh95
buffer: avoid creating unnecessary environment
e7e8256 
PR-URL: #58053
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@ghost ghost mentioned this pull request Jun 8, 2025
Closed
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

@legendecas legendecas legendecas approved these changes

@juanarbol juanarbol juanarbol approved these changes

@marco-ippolito marco-ippolito marco-ippolito 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++. 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.

7 participants

@anonrig @nodejs-github-bot @jasnell @ronag @legendecas @juanarbol @marco-ippolito