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

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

Open
1 task done
Tracked by #501
kwasimensah opened this issue May 10, 2022 · 3 comments
Open
1 task done
Tracked by #501
Labels
Bug thing that needs fixing semver:major backwards-incompatible breaking changes
Milestone

Comments

@kwasimensah
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

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

  • npm:
  • Node: v16
  • OS: Mac Os 12.3.1
  • platform: Macbook Pro
@kwasimensah kwasimensah added Bug thing that needs fixing Needs Triage needs an initial review labels May 10, 2022
@ljharb
Copy link

ljharb commented May 10, 2022

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
Copy link
Author

kwasimensah commented May 10, 2022 via email

@lukekarrys lukekarrys added semver:major backwards-incompatible breaking changes and removed Needs Triage needs an initial review labels Oct 27, 2022
@lukekarrys
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing semver:major backwards-incompatible breaking changes
Projects
None yet
Development

No branches or pull requests

4 participants