[WIP] implement group_voids
#2945
Conversation
|
|
Not keen on this approach. The AST shouldn't be altered in the |
|
@kzc I understand your viewpoint - any idea how to separate part of this workload to Bear in mind that the other attempts always end up with measurably worse |
slightly less improved gzip results
Why is it that creating a new variable ( |
|
If I know the underlying cause to that question other than the observable facts, I would have more luck with #2219 😅 |
|
My best guess so far is because variable name assignments from |
|
@kzc commented that this type of change can mess with the JIT, since this change introduces capturing of variables. I think that means that |
@alexlamsl Has this actually been tested? None of the experiments I did introduce capturing variables between functions (which is the reason why my |
I am referring to all the attempts under #2912 - capturing variable or not is moot, because it's the variable name assignment which is causing the |
|
I'm not concerned about theoretical or version-to-version variations on runtime performance - correctness and uglified sizes are the primary goals. |
|
Through trial an error some undefined seed symbol can be formulated in In my opinion some marginal optimizations are not worth pursuing if they negatively impact the structure and understandability of the code if there's any hope anyone other than you will produce PRs or maintain this code in the future. |
|
@alexlamsl Maybe we're talking past one another? My understanding is that your code reuses the same variable, while all my experiments were introducing a new variable per scope. // INPUT
function a(b) {
var x = 1;
if (b) return void 0;
return function () { return void 0; };
}
// OUTPUT
// alexlamsl's version
function a(b) {
var x = 1, u;
if (b) return u;
return function () { return u; };
}
// Skalman's version
function a(b) {
var x = 1, u;
if (b) return u;
return function () { var u2; return u2; };
} |
I assumed that this PR works within individual function scopes. If it introduces captured variables then I'd agree it shouldn't be enabled by default. |
@Skalman I understand what you are talking about, and I'm pointing out that it's not the culprit for the |
|
@alexlamsl I'd assume that the culprit is adding all the new
I don't really understand how this works, but I'll try it when I have time (another day). |
|
@kzc the scope of variable injection doesn't matter much - I didn't go down to individual What I am hoping to work out is, based on the fact that this PR works, what we might be able to learn about |
I have investigated this possibility before and it wasn't the case AFAICT. |
I agree it would be fine to improve |
I think we are on the same page. |
|
As for any concerns with performance differences: --- a/test/jetstream.js
+++ b/test/jetstream.js
@@ -96,7 +96,7 @@ if (typeof phantom == "undefined") {
page.open(url, function(status) {
if (status != "success") phantom.exit(1);
page.evaluate(function() {
- JetStream.switchToQuick();
+ JetStream.switchToLong();
JetStream.start();
});
});No statistically significant difference is measured between |
fixes #2585
/cc @kzc @Skalman