perf: save some bytes with more concise comparisons #10124
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Implicit boolean casts have a performance penalty, I think some of the flag checks where it is easily visible that the output is a number (& and --) can be shortened, others would probably have to be left as is I'd rather not get into golfing until we're close to release |
|
Hmm. I hadn't considered that. This PR makes the client folder go from 222.7 kB to 220.3 kB. That's without being compiled, minimized, etc. Although very few of these changes could be done by the minimizer, so it gives an idea of what the savings is. As a percentage, I imagine the savings would be larger when you minimize. I'll have to do some more precise benchmarking later, but I'd image this PR is probably in the realm of 200ms savings from the JS arriving sooner on a fast 3g connection. You'd have to do an awful lot of implicit conversions to get anywhere near that. |
Hmm, I guess it'd also reduce the decoding/parsing time somewhat, if it has a significant reduction in load times but hits perf, we could pick out hot paths to revert later |
|
I just tested this in the preview site and the difference is much smaller when gzipped. It's still hard to tell what the difference would be in an actual site because the preview site doesn't bundle or minimize the files. With minimization the difference would be a bit smaller and with bundling the difference would very likely be larger since you wouldn't load the files in parallel though it'd also compress more efficiently. So I'm guessing the difference in bandwidth is probably somewhere in the 10-40ms range. I'm guessing this PR would still be a win most of the time unless you've got a really compute heavy task. Maybe @trueadm could check whether it affects |
|
I'm not super happy with the changes. If we are going to code-golf, then we should do so without impacting performance. FWIW Preact undertook this path and they incurred a performance penalties from doing so, however they deemed it was worth it to meet their 3kb target. I wonder if @localvoid has opinions on the impact of these changes too. If we are to code-golf, then maybe we can leverage utility functions for these checks rather than making things implicit boolean casts. i.e. rather than doing |
Will the overhead of a function call really be faster than the implicit conversion? |
|
@dummdidumm It can be, however in the case of flags checks it looks to be almost as fast to do |
|
I'd be amazed if those utility functions were worth it after gzip compared to just doing the comparison directly. In general I think I prefer the more explicit thing ( |
|
I don't know how stable these benchmark results are or how many times you have to run it to be assured of the results. I ran it three times and got three different orderings - though it seemed that the function call generally made things slower. I copied it into jsbench.me, which runs more iterations and they were all basically equivalent:
So I think I'd optimize for size and readability over execution speed. Readability is somewhat subjective. My personal default is to write code in this implicit manner, which is why the extra code for the checks jumped out at me, but I know rich said he prefers the more explicit version. In terms of size though, I think this implicit version is pretty clearly the winner and while a few hundred bytes isn't enormous, just a couple changes like this add up to enough latency that users can tell the difference. |
| */ | ||
| function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, reconcile_fn) { | ||
| const is_controlled = (flags & EACH_IS_CONTROLLED) !== 0; | ||
| const is_controlled = !!(flags & EACH_IS_CONTROLLED); |
There was a problem hiding this comment.
If is_controlled is only ever used in contexts where its truthiness matters rather than it being true or false (I don't know whether this is the case), then we could remove the !! as well.
There was a problem hiding this comment.
Oh, I see now this was referenced in #10124 (comment) - Perhaps we don't want to make that change.
There was a problem hiding this comment.
usually where I left !! it was to appease typescript in a quick an minimal way
There was a problem hiding this comment.
@benmccann Have you tested Boolean(flags & EACH_IS_CONTROLLED) both in the runtime micro benchmarks and the gziped size? In term of readability I find the intent much more explicit that the !!() conversion, but I don't know about performance.
There was a problem hiding this comment.
To be honest the biggest culprit for perf in my findings wasn't the boolean conversion on flags, but rather on null values. If you look at the bytecode generated by V8, it's almost twice as many operations needed when doing boolean conversion on these fields (interestingly). So I'd be more leaning towards having an is_null function there, which is readable and also very fast.
There was a problem hiding this comment.
Hmm. But the "hybrid" option from the benchmark is consistently the slowest or 2nd slowest one for me, so I'm not sure I buy that it's faster and it's certainly much harder to write
There was a problem hiding this comment.
It was the fastest for me consistently with the first one. I also find it fine to write – I don't find writing explicit code to be of any benefit, especially if the type system isn't there to help you.
|
We can just look at the generated bytecode and assembly: function basic(i) {
if (i & 1) {
return 10;
}
return 20;
}
function negate(i) {
if (!!(i & 1)) {
return 10;
}
return 20;
}
function strictEq(i) {
if ((i & 1) !== 0) {
return 10;
}
return 20;
}Bytecode for
And after JIT compiler optimizations, all this functions will produce extactly the same assembly: So, in use cases like In use cases when we are passing boolean values as arguments to a function and don't want to rely on inlining heuristics, I would prefer to use explicit checks like function explicit(i) {
if (i === true) {
return 10;
}
return 20;
}
function implicit(i) {
if (i) {
return 10;
}
return 20;
}Bytecode for Bytecode for Generated assembly for Generated assembly for In use cases like I don't think that there should be a one-size-fits-all rule for comparisons, it should be evaluated for each case individually. |
|
@localvoid Thanks for the awesome response. To clarify too, with |
|
I think we can maybe do the flags checks using shorter boolean coercion (without the |
I made sure to review each change on a case-by-case basis rather than doing a find and replace for this reason. The vast majority of |
|
Given the concerns about performance regressions, and the fact that this code is harder to understand (since |
I don't know if this is someone's coding style, but figured it was probably leftover from the TypeScript conversion. I only updated the code that gets shipped to the client and didn't bother touching the compiler code at all