Closed XaveScor closed 10 months ago
Merging #23 into master will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #23 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 22 22
=========================================
Hits 22 22
Impacted Files | Coverage Δ | |
---|---|---|
src/index.js | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update c5b2b21...064a2e5. Read the comment docs.
I run the bench for current master and my PR separately and found some perf regression
Master:
# Strings
clsx x 8,041,328 ops/sec ±0.63% (91 runs sampled)
# Objects
clsx x 5,900,388 ops/sec ±1.84% (88 runs sampled)
# Arrays
clsx x 5,443,679 ops/sec ±1.21% (89 runs sampled)
# Nested Arrays
clsx x 4,078,254 ops/sec ±1.87% (86 runs sampled)
# Nested Arrays w/ Objects
clsx x 4,239,252 ops/sec ±2.31% (84 runs sampled)
# Mixed
clsx x 4,817,408 ops/sec ±1.43% (90 runs sampled)
# Mixed (Bad Data)
clsx x 1,638,964 ops/sec ±0.96% (89 runs sampled)
PR:
# Strings
clsx x 7,336,325 ops/sec ±1.83% (87 runs sampled)
# Objects
clsx x 5,717,162 ops/sec ±2.41% (83 runs sampled)
# Arrays
clsx x 5,551,438 ops/sec ±1.53% (89 runs sampled)
# Nested Arrays
clsx x 4,098,543 ops/sec ±1.70% (85 runs sampled)
# Nested Arrays w/ Objects
clsx x 4,149,760 ops/sec ±2.45% (84 runs sampled)
# Mixed
clsx x 4,865,544 ops/sec ±1.20% (88 runs sampled)
# Mixed (Bad Data)
clsx x 1,572,783 ops/sec ±1.56% (86 runs sampled)
Is this tradeoff applicable?
A variant I did:
function toVal(mix) {
var k, y, str='';
if (typeof mix === 'string' || typeof mix === 'number') {
return mix;
}
if (typeof mix === 'object') {
if (Array.isArray(mix)) {
for (y=0; y < mix.length; y++) {
if (mix[y]) {
if (k = toVal(mix[y])) {
str && (str += ' ');
str += k;
}
}
}
return str;
} else {
for (k in mix) {
if (mix[k]) {
str && (str += ' ');
str += k;
}
}
return str;
}
}
}
export function clsx() {
var i=0, tmp, x, str='';
while (i < arguments.length) {
if (tmp = arguments[i++]) {
if (x = toVal(tmp)) {
str && (str += ' ');
str += x
}
}
}
return str;
}
export default clsx;
Moving return str
to BOTH if
and else
parts of toVal()
shaved off additional 3 bytes!
i've small little suggestion tho... why not
var k, y, str='', a = typeof mix;
if (a === 'string' || a === 'number') {}
if (a === 'object') {}
@AzrizHaziq Believe or not, this does not make the Gzipped version smaller
Thanks for this PR & your patience.
I know it's been a while & there are now conflicts, but there were 2 points raised & I'll address them here rather than continue the inline conversations:
return val; // number
isn't a regression in tests/output, it's a regression in the function signature itself. Because the function can now sometimes return numbers instead of only strings, it's now polymorphic which is a deopt in V8 & therefore less performant. The benchmark results posted don't show this because there are no numbers in the benchmark itself.y
variable here – but only because k
is reserved for number types.Thanks again!
Size before:
Size after:
Perf on my Dell xps 13 9360 Core-i78550U: