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

__INLINE__ may be weaker than expected #656

Open
gdh1995 opened this issue Apr 22, 2020 · 7 comments
Open

__INLINE__ may be weaker than expected #656

gdh1995 opened this issue Apr 22, 2020 · 7 comments
Labels
compress Issue in the compression step suboptimal-output

Comments

@gdh1995
Copy link
Contributor

gdh1995 commented Apr 22, 2020

Bug report or Feature request? bug report

Version 4.6.11

Complete CLI command or minify() options used

npx terser -b -m -c inline=3,keep_fnames=false

terser input

var test = (function () {
    var isEnabled_ = 0, isLocked_ = 0;
    function setVEnabled(newEnabled) { isEnabled_ = newEnabled; }
    function setStatusLocked(newLocked) { isLocked_ = newLocked; }
    return [function (newEnabled, initing) {
        isEnabled_++
        /*#__INLINE__*/ setVEnabled(newEnabled);
        if (initing) {
            /*#__INLINE__*/ setStatusLocked(newEnabled);
        }
    }, function () { return [isEnabled_, isLocked_] }]
}());

terser output or error

var test = function() {
    var n = 0, t = 0;
    return [ function(r, u) {
        n++, function(t) {
            n = t;
        }(r), u && (t = r);
    }, function() {
        return [ n, t ];
    } ];
}();

Expected result

var test = function() {
    var n = 0, t = 0;
    return [ function(r, u) {
        n++, n = r, u && (t = r);
    }, function() {
        return [ n, t ];
    } ];
}();

Extra info

If keep_fnames=true, then both the two setter functions are kept.

My project, Vimium C, is turning to ES6 modules, and I've written quite a lot of "setter" functions like:

https://github.com/gdh1995/vimium-c/blob/7329b6a1179c4d3e2bde5617ba878ef954a213d7/content/scroller.ts#L56-L68

Although I've added some /*#__INLINE__*/ to those function calls, some are still not inlined prefectly. The below is an example of generated code:
image

@fabiosantoscode
Copy link
Collaborator

fabiosantoscode commented Apr 23, 2020

I see what's happening here.

Terser's not inling because it sees a name collision with newEnabled.

I'm really not comfortable inlining this, since inlining too much has been at the core of some of the hardest problems I've had to deal with in Terser.

I tried to tackle issues like this in #661, where I thought of renaming every variable in an inlined function to prevent any and all conflicts, but it turned out to be pretty complicated.

@gdh1995
Copy link
Contributor Author

gdh1995 commented Apr 23, 2020

Um, what can I do to work around it? I mean I can modify my source code, or write code to transform AST trees generated by terser and then pass it into terser to compress it.

BTW, I also wonder if there's a directive in comments to force terser not to inline constant string variables in closures. I extracts some constants into such outside variables to reduce code size, but terser always "inlines" them...

@gdh1995
Copy link
Contributor Author

gdh1995 commented Apr 23, 2020

Well, I add a pre-processor to replace all setter functions by replacing set\$\w+\(...\) with $name = ..., and then this issue is bypassed. For who are interested in the details, you may read https://github.com/gdh1995/vimium-c/blob/2401fcf6bb576eb13035f9fa88124bc93978ea66/scripts/dependencies.js#L383 .

@gdh1995 gdh1995 closed this as completed Apr 23, 2020
@fabiosantoscode
Copy link
Collaborator

If your setters are automatically generated, you can get around this easily by making the name of the setter's argument different from names which might be used.

I might revisit my decision of not doing automatic renaming, now that there's a nice way of knowing whether mangle is enabled from inside the compressor.

@gdh1995
Copy link
Contributor Author

gdh1995 commented Apr 23, 2020

The name are hand-written by myself. Today I once tried using some unique (very long) argument names, but it failed and there were still some embeded functions. I didn't do much more test and just switched to pre-processing.

@fabiosantoscode
Copy link
Collaborator

You should try using more passes, on top of your unique argument names. Further passes should be a second opportunity for Terser to inline.

@gdh1995
Copy link
Contributor Author

gdh1995 commented Apr 28, 2020

Oh I think I've found why my code could not be completely inlined: sometimes I call set_***(...) twice inside a same function (block).

For example, in such code the second set_isHintingInput can not be inlined as expected:

/*#__INLINE__*/ set_isHintingInput(1);
select_(hints2[sel].d, null, false, action);
/*#__INLINE__*/ set_isHintingInput(0);

And if a first call is in a closure, and the second call occurs in an inner function, then the second will also be kept.

In further tests, I found both passes=4 and calling terser.minify for 4 times would fail, so it seems because of "a prior assignment to the target variable".

Version 5.7.1

Complete CLI command or minify() options used

npx terser -f beautify -m -c inline=3,keep_fnames=false

terser input

var test = (function () {
    var isEnabled_ = 0;
    function setVEnabled(newEnabled) { isEnabled_ = newEnabled; }
    return [function (myEnabled, initing) {
        /*#__INLINE__*/ setVEnabled(myEnabled);
        window.foo(myEnabled, initing);
        /*#__INLINE__*/ setVEnabled(myEnabled);
    }, function () { return isEnabled_ }]
}());

terser output or error

var test = function() {
    var n = 0;
    return [ function(t, o) {
        n = t, window.foo(t, o), function(t) {
            n = t;
        }(t);
    }, function() {
        return n;
    } ];
}();

Expected result

var test = function() {
    var n = 0;
    return [ function(t, o) {
        n = t, window.test(t, o), n = t;
    }, function() {
        return n;
    } ];
}();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compress Issue in the compression step suboptimal-output
Projects
None yet
Development

No branches or pull requests

3 participants