@ljharb You have an interesting point, if theparseOptions was written just because of this function, so yes, maybe the necessity of parsing the options is just needed to check if is not undefined/null or bollean, In the other cases, we can just use the same object.

About the perf, I think it will not affect us since we already look for properties in the object that could doesn't exist, and if we just performObject.freeze for an empty object andloose, we still save memory and avoid doing more comparisons and reduce the maintenance.

rtl is only used infunctions/coerce.js, a standalone function not reused in any other part of semver.

This refactor is great because our coverage tests are exposing the fact that we are evaluating for a parameter that doesn't matter to this class. The flag doesn't affect the data being memoized so it should not need to be accounted for in the memo key.

parseOptions is doing two things, it is allowing for theoptions parameter to overload as a boolean representingloose, and it is also ensuring that the object keys are in a deterministic order.

I searched through the code to see ifoptions was doing any other heavy lifting and found:

• classes/comparator.js#intersects recreating the logic instead of callingparseOptions
• functions/inc.js making the positional parameteroptions optional by testing if it is a string, this likely would only be affected when interpreting params from the cli, which already builds up an options object and only uses that. I don't think we need to support passing'true' for theloose paramater.
• ranges/subset.js looks atoptions.includePrerelease without having cleaned it viaparseOptions

Nothing I found would indicate that the ordering of parseOptions matters except in this one case where we are using it to memoize a cache key. I think that memo logic should be isolated to where it's used, and made to only account for flags that affect the results.

What happens if another option is added later?

If those options affect whatclasses/range.js returns there will need to be tests that cover those lines, which will fail if we are improperly memoizing the results.

So far so good imho. I am going to ping@kurtextrem and@jakebailey since they were pretty well engaged with the other performance PR. If either of you don't want me to ping you on this and the next performance PR(s) please let me know.

Always happy to be pinged 😃

I'll test these PRs out on my super stressful DefinitelyTyped monorepo case once I'm at my desk.

• classes/comparator.js#intersects recreating the logic instead of callingparseOptions

I will push a fix to this.

• functions/inc.js making the positional parameteroptions optional by testing if it is a string, this likely would only be affected when interpreting params from the cli, which already builds up an options object and only uses that. I don't think we need to support passing'true' for theloose parameter.

I think the logic behind the testing is when we send theidentifier onoptions, like:

inc(version: string,release: string,options: any,identifier: string);inc(version: string,release: string,identifier: string);

• ranges/subset.js looks atoptions.includePrerelease without having cleaned it viaparseOptions

I already fix this case in this PR by callingparseOptions in the beginning.

Nothing I found would indicate that the ordering of parseOptions matters except in this one case where we are using it to memoize a cache key. I think that memo logic should be isolated to where it's used, and made to only account for flags that affect the results.

In this case, should I remove all the object freeze from the other PR and change the tests to accept any object?

If those options affect what classes/range.js returns there will need to be tests that cover those lines, which will fail if we are improperly memoizing the results.

So currently we don't need any additional test, right?

In this case, should I remove all the object freeze from the other PR and change the tests to accept any object?

I haven't looked at the other PR yet. Let's talk about that over there though.

So currently we don't need any additional test, right?

Correct, imho. We would need new tests in the event that we added new options, at which point if we didn't also update the memoization they would fail.

I already fix this case in this PR by calling parseOptions in the beginning.

I don't think this is fixed though.subset is exported directly fromindex.js so folks calling it are sending an unparsedoptions param directly to code that evaluatesoptions.includePrerelease without first having ran it throughparseOptions.

this is much simpler and thus much less risky, well done

this is much simpler and thus much less risky, well done

Agreed. The past two weeks@H4ad has been extremely patient and diligent responding to all of the feedback in the PRs they've made. The end results have been worth it and I am thankful for the effort being made here.

I've given this a 👍 and will let it sit now through the weekend to give others time to catch up.

Here's my DT stress test using pnpm. Before:

Done in 2m 14.5stotal time:  134.82suser time:   162.81ssystem time: 31.61sCPU percent: 144%max memory:  1752 MB

This PR ("semver": "github:h4ad-forks/node-semver#fix/better-cache-key-factory"):

Done in 2m 13stotal time:  133.25suser time:   161.42ssystem time: 31.23sCPU percent: 144%max memory:  1768 MB

So, seemingly no impact. This is probably not showing much improvement becuase I went and added a Range cache to pnpm (which yarn berry also has). I can try removing that cache and show before/after this PR, though, if people are interested.

Eh, I just ran it anyway.

Here's pnpm without the Range cache, instead usingsemver.satisfies like it used to:

Done in 2m 43stotal time:  163.27suser time:   194.27ssystem time: 31.76sCPU percent: 138%max memory:  1792 MB

Already a lot slower than my baseline. With this PR, it gets a little better:

Done in 2m 37.5stotal time:  157.75suser time:   189.12ssystem time: 31.90sCPU percent: 140%max memory:  1771 MB

Which, is still a light improvement, but not game changing like the options PR for pnpm anyway. Not to say this isn't a good change; absolutely take it 😄

Thanks@jakebailey it's good to have benchmarks that use a whole program. Sometimes an optimization helps a use case that isn't very common.

The primary benefit of this PR is that it sets up the parse options refactor, since the memoization is now decoupled from that function.

From my side, I would also say lgtm, even if it might not increase performance drastically, as it gives a good baseline in case in the future more options are added or for using more bit masks inside this module.

@H4ad looks like#530 conflicted w/ this PR. Once that is resolved I'll land this.

@wraithgar Fixed!