Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance unused #4858

Merged
merged 1 commit into from
Apr 22, 2021
Merged

enhance unused #4858

merged 1 commit into from
Apr 22, 2021

Conversation

alexlamsl
Copy link
Collaborator

@kzc this addresses the first two cases in #4855 (comment)

That last one is trickier since it relies on join_vars:

$ echo 'var F={}; F.a=0; F.b=1; F.c=2;' | uglify-js -bc toplevel
 
$ echo 'var F={}; F.a=0; F.b=1; F.c=2;' | uglify-js -bc toplevel,join_vars=0
var F = {};

F.a = 0, F.b = 1, F.c = 2;

which obviously won't work with self references.

I think to make that case work would require extending reduce_vars to cover object mutation in a similar manner to constant / class / function

@kzc
Copy link
Contributor

kzc commented Apr 21, 2021

A one liner - well done!

Handling the object literal self reference would be gravy. I haven't seen it in the wild.

@alexlamsl
Copy link
Collaborator Author

... except there's corner case to be fixed − will take a look after 💤

@alexlamsl alexlamsl changed the title enhance unused [WIP] enhance unused Apr 21, 2021
@kzc
Copy link
Contributor

kzc commented Apr 21, 2021

The third party application tests don't trigger a failure often, but are quite useful when they do.

I think ufuzz is capable of creating property cycles in data structures.

@alexlamsl
Copy link
Collaborator Author

A one liner - well done!

For every problem there is a solution that is simple, neat − and wrong.

@alexlamsl alexlamsl changed the title [WIP] enhance unused enhance unused Apr 21, 2021
@kzc
Copy link
Contributor

kzc commented Apr 21, 2021

As luck would have it, I know of a tool that can transform any JS into a one-liner.
;-)

@kzc
Copy link
Contributor

kzc commented Apr 21, 2021

Do you have a reduced test case for the web-tooling-benchmark.sh failure?

@alexlamsl
Copy link
Collaborator Author

alexlamsl commented Apr 21, 2021

Do you have a reduced test case for the web-tooling-benchmark.sh failure?

TBH I haven't even tried to reproduce the failure locally, since I've already figured out the logical fallacy before I woke up and rewrote the patch.

So if the build testing fails again that's the first thing I'll do 👻

@alexlamsl
Copy link
Collaborator Author

Been busy filtering through all the failure reports from GitHub up until recently.

https://www.githubstatus.com/incidents/cj7gzzj30411

(Yeah, it lasted much longer than claimed, but they've been like that for a while now...)

@alexlamsl
Copy link
Collaborator Author

@alexlamsl alexlamsl merged commit bddb5a0 into mishoo:master Apr 22, 2021
@alexlamsl alexlamsl deleted the unused-self-ref branch April 22, 2021 02:58
@kzc
Copy link
Contributor

kzc commented Apr 22, 2021

@alexlamsl Here's another optimization for you to consider...

The alias y below is optimized away correctly with collapse_vars:

$ echo 'var x = Math.random(), y = x; console.log(x/y, y/x, (x+y)/x);' | uglify-js -c toplevel
var x=Math.random();console.log(x/x,x/x,(x+x)/x);

But if the code is altered slightly then the alias y will not be optimized away:

$ echo 'var x, y = x = Math.random(); console.log(x/y, y/x, (x+y)/x);' | uglify-js -c toplevel
var x,y=x=Math.random();console.log(x/y,y/x,(x+y)/x);

If this optimization were to be implemented then the generated ES5 code at the end of this comment:

!function() {
  var e, t, r = {},
    n = {};
  t = n = function(e, t) {
    return t.get ? t.get.call(e) : t.value
  }, n.default = t, n.__esModule = true;
  var u, a = n,
    o = {};
  u = o = function(e, t, r) {
    if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance");
    return t.get(e)
  }, o.default = u, o.__esModule = true;
  var l = o;
  e = r = function(e, t) {
    var r = l(e, t, "get");
    return a(e, r)
  }, r.default = e, r.__esModule = true
}();

could be completely optimized away with uglify-js -c merge_vars=false,passes=9 aided by #4858.

After pass 1 the unneeded assignments r = {}, n = {} and o = {} would be dropped by reduce_vars leaving all top level declared variables in the example as single assignment.

I suspect that merge_vars may be at odds with this proposed optimization since it would complicate the analysis if the aliased single assigned variables are repurposed in the example above. Or perhaps not.

@alexlamsl
Copy link
Collaborator Author

Taught collapse_vars to handle:

$ echo 'var x, y = x = Math.random(); console.log(x/y, y/x, (x+y)/x);' | uglify-js -bc toplevel
var x;

x = Math.random();

console.log(x / x, x / x, (x + x) / x);

But that still leave your larger example with:

$ uglify-js test.js -bc merge_vars=false,passes=9
!function() {
    function n(e, t) {
        return t.get ? t.get.call(e) : t.value;
    }
    function o(e, t, r) {
        if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance");
        return t.get(e);
    }
    (n.default = n).__esModule = !0;
    var a = n;
    (o.default = o).__esModule = !0;
    var r, l = o;
    ((r = function(e, t) {
        var r = l(e, t, "get");
        return a(e, r);
    }).default = r).__esModule = !0;
}();

@kzc
Copy link
Contributor

kzc commented Apr 22, 2021

The impasse appears to be the remaining aliases a = n and l = o and the nested assignment of r. If the code is rewritten as the following, it optimizes nicely...

$ cat test.js
!function() {
    function n(e, t) {
        return t.get ? t.get.call(e) : t.value;
    }
    function o(e, t, r) {
        if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance");
        return t.get(e);
    }
    (n.default = n).__esModule = !0;
    (o.default = o).__esModule = !0;
    var r = function(e, t) {
        var r = o(e, t, "get");
        return n(e, r);
    };
    (r.default = r).__esModule = !0;
}();

$ cat test.js | uglify-js -bc

$ 

Some optimizations prevent other optimizations from occurring. It is a difficult balancing act. It wouldn't help in this case, but there might be heuristics that could be employed such as: If passes > 1 then merge_vars could only run on the final pass, for example.

As far as alias dropping goes, if one variable mirrors another and neither have subsequent assignments then the original variable could be used for the alias. No doubt there will be complications in practise. The devil is in the details.

@alexlamsl
Copy link
Collaborator Author

Some optimizations prevent other optimizations from occurring. It is a difficult balancing act.

My plan is just to identify those and generalise the impeded option, e.g. reduce_vars can now handle multiple assign-read cycles within a single variable, so merge_vars won't interfere with its operations.

The impasse appears to be the remaining aliases a = n and l = o and the nested assignment of r. If the code is rewritten as the following, it optimizes nicely...

Thanks for the analysis − will take a closer look when I re-emerge from my alcove...

@kzc
Copy link
Contributor

kzc commented Apr 22, 2021

This seems to be an obstacle to further optimization...

Compare this code:

$ cat r1.js

    effect();
    var r;
    r = function(e, t) {
        var r = o(e, t, "get");
        return n(e, r);
    };
    (r.default = r).__esModule = !0;
$ cat r1.js | uglify-js -c toplevel,passes=9
var r;effect(),((r=function(e,r){return r=o(e,r,"get"),n(e,r)}).default=r).__esModule=!0;

to:

$ cat r2.js

    effect();
    var r = function(e, t) {
        var r = o(e, t, "get");
        return n(e, r);
    };
    (r.default = r).__esModule = !0;
$ cat r2.js | uglify-js -c toplevel
effect();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants