Skip to content

lib: refactor to use more primordials in internal/freelist.js - #36565

Closed
Lxxyx wants to merge 1 commit into
nodejs:mainfrom
Lxxyx:lib-freelist-use-primordials
Closed

lib: refactor to use more primordials in internal/freelist.js#36565
Lxxyx wants to merge 1 commit into
nodejs:mainfrom
Lxxyx:lib-freelist-use-primordials

Conversation

@Lxxyx

Copy link
Copy Markdown
Member

Reference

The file is used here:

constFreeList=require('internal/freelist');

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@mscdex

Copy link
Copy Markdown
Contributor

The benchmark CI regression is quite significant:

 confidence improvement accuracy (*) (**) (***)
misc/freelist.js n=100000 *** -73.53 % ±1.13% ±1.52% ±2.01%

@ghostghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@Lxxyx

Copy link
Copy Markdown
MemberAuthor

The benchmark CI regression is quite significant:

 confidence improvement accuracy (*) (**) (***)
misc/freelist.js n=100000 *** -73.53 % ±1.13% ±1.52% ±2.01%

freelist is simple data structure, so refactoring to primordials causes a significant performance regression. I tried some solutions, but they didn't work. Do you all have any ideas about this issue? @aduh95

@ExE-Boss

Copy link
Copy Markdown
Contributor

I’m going to try implementing SafeArray.

@ExE-Boss

Copy link
Copy Markdown
Contributor

I’ve now opened #36600, which implements SafeArray.

@nodejsnodejs deleted a comment Dec 23, 2020
@nodejsnodejs deleted a comment Dec 23, 2020
@targostargos added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 27, 2020
@aduh95aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 19, 2023
@Lxxyx

Copy link
Copy Markdown
MemberAuthor

Close due to long-term inactivity

@LxxyxLxxyx closed this Mar 10, 2025
ExE-Boss added a commit to ExE-Boss/node that referenced this pull request Jul 28, 2026
lib: add `SafeArray` to `primordials` and use it for `FreeList`
Refs: nodejs#36304 (comment)
Refs: nodejs#36565
Signed-off-by: ExE Boss <ExE-Boss@code.ExE-Boss.tech>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.needs-benchmark-ciPR that need a benchmark CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Lxxyx@mscdex@ExE-Boss@targos@aduh95