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

lib: ensure no holey array in fixed_queue #54537

Merged
merged 2 commits into from
Sep 2, 2024
Merged

lib: ensure no holey array in fixed_queue #54537

merged 2 commits into from
Sep 2, 2024

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Aug 24, 2024

Fixes: #54472
Refs: #54186 (comment)

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 24, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (e70bd47) to head (348536f).
Report is 93 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54537      +/-   ##
==========================================
- Coverage   87.34%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182524   182621      +97     
  Branches    35026    35044      +18     
==========================================
+ Hits       159420   159501      +81     
- Misses      16373    16395      +22     
+ Partials     6731     6725       -6
Files with missing lines Coverage Δ
lib/internal/fixed_queue.js 100.00% <100.00%> (ø)

... and 48 files with indirect coverage changes

@RedYetiDev RedYetiDev added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 24, 2024
@RedYetiDev
Copy link
Member

Hi! Could you amend your commit message to begin with an active verb (after the subsystem)?

@jakecastelli
Copy link
Contributor

jakecastelli commented Aug 25, 2024 •
edited
Loading

I have run the benchmark for async_hooks and events, here are the results:

events: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1616/
async_hooks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1615/

Does not look like this change would cause any performance regression. But would be happy to know if any benchmark that is missed.

@jazelly jazelly force-pushed the no-holey-array-in-fixed-queue branch 2 times, most recently from 4bee60e to fbe2d3a Compare August 25, 2024 03:14
@jazelly jazelly changed the title lib: no holey array in fixed_queue lib: ensure no holey array in fixed_queue Aug 25, 2024
@jazelly @jakecastelli
lib: ensure no holey array in fixed_queue
5f3a09d 
Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com>
@jazelly jazelly force-pushed the no-holey-array-in-fixed-queue branch from fbe2d3a to 5f3a09d Compare August 25, 2024 03:16
@jazelly
Copy link
Contributor Author

jazelly commented Aug 25, 2024 •
edited
Loading

PR comments addressed. I have also updated the data structure comments in fixed_queue as it's no longer with [empty]

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024
@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
@jakecastelli jakecastelli self-assigned this Aug 26, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 26, 2024 •
edited by jakecastelli
Loading

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

@jakecastelli
Copy link
Contributor

jakecastelli commented Aug 26, 2024 •
edited
Loading

@joyeecheung @targos @benjamingr PTAL 🙏

@benjamingr
Copy link
Member

I'm approving this even without a test but a test would be nice

@jazelly
Copy link
Contributor Author

jazelly commented Aug 26, 2024 •
edited
Loading

@benjamingr see this thread. I think (and I agree) the idea is that we don't want to show it as a wanted behaviour. We fix it to avoid hard crashes.

@jakecastelli
Copy link
Contributor

a test would be nice

Thinking about this again, maybe we should add a test to the test/known_issues and put refs on the top. In this way, folks who read the test would not think this is a supported behaviour. Wdyt @jazelly

@jazelly
Copy link
Contributor Author

jazelly commented Aug 27, 2024

Oh, I didn't know there are test/known_issues. I think it makes sense to place it there with a comment.

@jazelly
Copy link
Contributor Author

jazelly commented Aug 27, 2024

Actually, this may not be a known issue anymore - especially with this patch. I added a test case in test-fixed-queue.js to assert the queue no longer being holey. PTAL

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

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 27, 2024 •
edited by jakecastelli
Loading

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

@jakecastelli
Copy link
Contributor

jakecastelli commented Aug 27, 2024 •
edited
Loading

Do you have any objections on this one? @joyeecheung @legendecas 👀

jakecastelli
jakecastelli approved these changes Sep 1, 2024
View reviewed changes
Copy link
Contributor

@jakecastelli jakecastelli left a comment

Choose a reason for hiding this comment

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

the added test LGTM

@jakecastelli jakecastelli added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 1, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 981c701 into nodejs:main Sep 2, 2024
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 981c701

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
@jazelly @jakecastelli @aduh95
lib: ensure no holey array in fixed_queue
2a1f56c 
Co-authored-by: Jake Yuesong Li <jake.yuesong@gmail.com>
PR-URL: #54537
Fixes: #54472
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedYetiDev RedYetiDev RedYetiDev left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos targos approved these changes

@jakecastelli jakecastelli jakecastelli approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees

@jakecastelli jakecastelli

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
7 participants
@jazelly @RedYetiDev @jakecastelli @nodejs-github-bot @benjamingr @jasnell @targos