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.

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

lib: inline Array operations in FreeList methods #37649

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 7, 2021

This inlines a simplified behaviour of %Array.prototype.pop% and %Array.prototype.push% in FreeList’s methods and sets the prototype of the list to null so that OrdinarySetWithOwnDescriptor doesn’t walk up the prototype chain.

This avoids depending on user code not mutating %Array.prototype%.pop and %Array.prototype%.push.


Refs: #36565
Refs: #36600

If I had to guess, I’d say V8 is doing some inlining shenanigans when it detects a push or pop method on an ordinary array with %Array.prototype% in its [[Prototype]] internal slot.


/cc @aduh95 @Lxxyx

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 7, 2021
@aduh95
Copy link
Contributor

aduh95 commented Mar 7, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/957/

Benchmark results still show regression:

                         confidence improvement accuracy (*)   (**)  (***)
misc/freelist.jsn=100000        ***    -86.53 %       ±1.04% ±1.40% ±1.84%

lib/internal/freelist.js Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the lib/freelist/inline-array-ops branch from f29ec6b to 326ddd9 Compare March 7, 2021 15:21
this.list.pop() :
ReflectApply(this.ctor, this, arguments);
const { list } = this;
const { length } = list;
Copy link
Contributor

Choose a reason for hiding this comment

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

You might see if avoiding the destructuring assignments helps with the perf regressions. I know at one point it was significantly slower.

@aduh95
Copy link
Contributor

aduh95 commented Mar 7, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/959/

                          confidence improvement accuracy (*)   (**)  (***)
 misc/freelist.jsn=100000        ***    -90.38 %       ±1.42% ±1.91% ±2.53%

@@ -9,18 +10,29 @@ class FreeList {
this.name = name;
this.ctor = ctor;
this.max = max;
this.list = [];
this.list = ObjectSetPrototypeOf([], null);
Copy link

@Mifrill Mifrill Oct 27, 2023

Choose a reason for hiding this comment

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

@ExE-Boss I see in #36600 you using SafeArray:
https://github.com/ExE-Boss/node/blob/283189fcce72f98f55d335292db0b3b77a252279/lib/internal/freelist.js#L13

To me it seems the intention is the same to ensures that the array used is not affected by any potential modifications to the standard Array.prototype in user code or other libraries. I think it's great, and this is about maintaining encapsulation and avoiding unexpected side effects caused by changes to global prototypes.

Could you please kindly clarify what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is largely performance, as #36600 has a -91% perf regression, and this only has a -90%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants