fix: faster paths for compare #813
Conversation
H4ad
force-pushed
the
perf/compare-main
branch
from
3a33b93 to
024ffda
Compare
|
I think this is worth digging into just a little bit. Under what situations is the identifier parseable as an integer but not cast as one? This internal function is only ever used in the semver class itself.
I think what we have here is a classic pitfall of unit testing: by testing the internals file directly we have disregarded the ways in which it can actually be called. Unless you start manually setting entries in the Unfortunately because of the coverage map.js file we can't do away with this test files itself. What we can do is write tests in that file that don't require the internals, but get to the code from the actual Semver object. TLDR: let's stop casting these things to numbers at all. We can remove const numeric = /^[0-9]+$/and const anum = numeric.test(a)
const bnum = numeric.test(b)
if (anum && bnum) {
a = +a
b = +b
}What do you think? |
|
Oh, I see it now. This is optimized for the absolute least common path possible. If you are comparing prerelease identifiers, and one is a number and the other is not, then we prefer the number. We do not need to be using this function at all on anything except the prerelease array. And then I think this function can be left as-is. |
|
|
|
Yeah Check it out: 03315c7 |
|
Another thing: prerelease portions are already cast as numbers if they're numbers. Lines 67 to 72 in 2471d75 The only reason we even need the re-cast back to a number is because build does not do this: Line 77 in 2471d75 Some consistency between these two approaches would mean a much more efficient |
H4ad
force-pushed
the
perf/compare-main
branch
from
024ffda to
114b57d
Compare
|
About your last comment, doing that change will not be a breaking change? If so, I would like to keep that in a different PR About this change, if we add a regex to test for build aswell, I thought it would affect the performance but I tried to convert the regexp to just +num and I didn't get any significant perf improvement: -parse(1.0.0) x 12,395,608 ops/sec ±0.30% (98 runs sampled)
+parse(1.0.0) x 12,014,007 ops/sec ±0.43% (94 runs sampled)
-parse(2.1.0) x 12,556,489 ops/sec ±0.57% (100 runs sampled)
+parse(2.1.0) x 12,337,414 ops/sec ±0.25% (98 runs sampled)
-parse(3.2.1) x 12,518,209 ops/sec ±0.33% (98 runs sampled)
+parse(3.2.1) x 11,770,690 ops/sec ±0.42% (98 runs sampled)
-parse(v1.2.3) x 12,264,666 ops/sec ±0.38% (96 runs sampled)
+parse(v1.2.3) x 11,690,669 ops/sec ±0.44% (97 runs sampled)
-parse(1.2.3-0) x 5,224,079 ops/sec ±0.42% (97 runs sampled)
+parse(1.2.3-0) x 5,635,402 ops/sec ±1.22% (93 runs sampled)
-parse(1.2.3-123) x 3,259,160 ops/sec ±0.82% (97 runs sampled)
+parse(1.2.3-123) x 3,677,786 ops/sec ±0.34% (96 runs sampled)
-parse(1.2.3-1.2.3) x 2,523,342 ops/sec ±0.40% (100 runs sampled)
+parse(1.2.3-1.2.3) x 3,021,260 ops/sec ±0.47% (100 runs sampled)
-parse(1.2.3-1a) x 4,114,595 ops/sec ±0.49% (91 runs sampled)
+parse(1.2.3-1a) x 3,956,804 ops/sec ±0.27% (98 runs sampled)
-parse(1.2.3-a1) x 4,147,904 ops/sec ±1.07% (98 runs sampled)
+parse(1.2.3-a1) x 4,173,043 ops/sec ±0.48% (96 runs sampled)
-parse(1.2.3-alpha) x 3,875,760 ops/sec ±1.09% (94 runs sampled)
+parse(1.2.3-alpha) x 3,923,173 ops/sec ±0.37% (96 runs sampled)
-parse(1.2.3-alpha.1) x 2,668,685 ops/sec ±0.84% (92 runs sampled)
+parse(1.2.3-alpha.1) x 2,754,900 ops/sec ±0.25% (98 runs sampled)
-parse(1.2.3-alpha-1) x 3,846,811 ops/sec ±1.10% (96 runs sampled)
+parse(1.2.3-alpha-1) x 3,899,305 ops/sec ±0.45% (96 runs sampled)
-parse(1.2.3-alpha-.-beta) x 2,788,394 ops/sec ±1.34% (97 runs sampled)
+parse(1.2.3-alpha-.-beta) x 2,579,693 ops/sec ±0.59% (96 runs sampled)
-parse(1.2.3+456) x 6,923,477 ops/sec ±0.61% (94 runs sampled)
+parse(1.2.3+456) x 7,217,113 ops/sec ±0.75% (97 runs sampled)
-parse(1.2.3+build) x 6,379,351 ops/sec ±0.89% (91 runs sampled)
+parse(1.2.3+build) x 6,937,235 ops/sec ±1.29% (95 runs sampled)
-parse(1.2.3+new-build) x 6,220,420 ops/sec ±1.42% (95 runs sampled)
+parse(1.2.3+new-build) x 6,836,206 ops/sec ±0.77% (95 runs sampled)
-parse(1.2.3+build.1) x 4,624,219 ops/sec ±0.46% (97 runs sampled)
+parse(1.2.3+build.1) x 5,122,833 ops/sec ±0.62% (94 runs sampled)
-parse(1.2.3+build.1a) x 4,366,437 ops/sec ±0.52% (95 runs sampled)
+parse(1.2.3+build.1a) x 4,391,526 ops/sec ±2.63% (91 runs sampled)
-parse(1.2.3+build.a1) x 4,316,662 ops/sec ±0.66% (94 runs sampled)
+parse(1.2.3+build.a1) x 4,131,423 ops/sec ±3.49% (89 runs sampled)
-parse(1.2.3+build.alpha) x 4,204,876 ops/sec ±0.96% (97 runs sampled)
+parse(1.2.3+build.alpha) x 4,486,159 ops/sec ±0.82% (98 runs sampled)
-parse(1.2.3+build.alpha.beta) x 3,773,754 ops/sec ±0.50% (89 runs sampled)
+parse(1.2.3+build.alpha.beta) x 4,049,244 ops/sec ±1.07% (92 runs sampled)
-parse(1.2.3-alpha+build) x 3,168,472 ops/sec ±0.36% (95 runs sampled)
+parse(1.2.3-alpha+build) x 3,193,028 ops/sec ±1.02% (94 runs sampled)Change: diff --git a/classes/semver.js b/classes/semver.js
index 92254be..9d18f21 100644
--- a/classes/semver.js
+++ b/classes/semver.js
@@ -64,10 +64,10 @@ class SemVer {
this.prerelease = []
} else {
this.prerelease = m[4].split('.').map((id) => {
- if (/^[0-9]+$/.test(id)) {
- const num = +id
- if (num >= 0 && num < MAX_SAFE_INTEGER) {
- return num
+ if (id !== '') {
+ const idNum = +id
+ if (idNum >= 0 && idNum < MAX_SAFE_INTEGER) {
+ return idNum
}
}
return id |
Yeah I don't know what it'd take to make them consistent, it's potentially breaking. We should probably put it in the breaking change backlog |
wraithgar
changed the title
Fun challenge: try to get |
A simple change to avoid Regex when we already know both values are number.