search

npm / node-semver

The semver parser for node (the one npm uses)
ISC License
5.06k stars 492 forks source link

[BUG] SemVer.compare causes spurious allocations of SemVer objects #458

Open kwasimensah opened 2 years ago

kwasimensah commented 2 years ago

Is there an existing issue for this?

Current Behavior

https://github.com/npm/node-semver/blob/main/functions/compare.js#L3 always creates a new SemVer objects even if you're careful about making sure you're already using SemVer objects.

It relies on https://github.com/npm/node-semver/blob/main/classes/semver.js#L14 to return the original semver from the constructor (TIL Javascript constructors can have return statements). This may work but is surprising and seems to cause a spurious allocation.

Expected Behavior

https://github.com/npm/node-semver/blob/main/functions/compare.js#L3 conditionally converts it's operands to SemVer objects and doesn't create spurious allocations.

Steps To Reproduce

const a = new SemVer("1.0");
const b = new SemVer("2.0");

// Chrome's Memory profiler will tell you that new SemVer objects we're created in this call.
a.compare(b);

Environment

ljharb commented 2 years ago

Why is this a problem? "allocations" aren't observable to JavaScript; are quite unique to individual implementations, and in this case can likely be trivially JITted away.

kwasimensah commented 2 years ago

I’m calling compare inside of what’s essentially a tight loop. The allocations are t being JITed away and garbage collection is showing up on my profiling so I’m trying to reduce allocations.

On Tue, May 10, 2022 at 1:30 AM Jordan Harband @.***> wrote:

Why is this a problem? "allocations" aren't observable to JavaScript; are quite unique to individual implementations, and in this case can likely be trivially JITted away.

— Reply to this email directly, view it on GitHub https://github.com/npm/node-semver/issues/458#issuecomment-1121948243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQGASC3DHRCWEN4HSCTDV3VJHX5TANCNFSM5VQJLF2A . You are receiving this because you authored the thread.Message ID: @.***>

lukekarrys commented 1 year ago

using this issue as a reference for auditing the creation of all new Semver/Range classes when called from methods. semver tries to reuse instances in some cases but not others. I think the ideal solution would be to have a standard way of determining options and always reusing the instance, plus the addition of a clone method (#378).