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

reduce_func should not be obsoleted #696

Closed
jareguo opened this issue May 13, 2020 · 7 comments
Closed

reduce_func should not be obsoleted #696

jareguo opened this issue May 13, 2020 · 7 comments

Comments

@jareguo
Copy link

jareguo commented May 13, 2020

Bug report or Feature request?

Feature request

Version (complete output of terser -V or specific git commit)

4.6.13

Complete CLI command or minify() options used

reduce_vars: true


Sorry to submit a new issue based on #350

As isiahmeadows mentioned:

Just a simple feature request that there should be an escape hatch for inline to force a function to not be inlined. Of course, this is almost never a good idea, but in performance-critical scenarios, there might be a reason to explicitly not inline a function, mainly to guide engines to inline the caller more easily.

And @fabiosantoscode mentioned:

If you want to see if that affects performance, you could run a benchmark comparing your code before and after reduce_vars.

So here is the benchmark:

https://jsperf.com/reduce-function-terser/1

image

As you can see, inlining function will cause performance degradation. Especially when the function is used as a constructor!
Performance is critical for us, but it's unwise for us to add /*@__NOINLINE__*/ everywhere. Can we just make it not inline by default? Thanks.

@jareguo jareguo changed the title reduce_func should not obsoleted reduce_func should not be obsoleted May 13, 2020
@fabiosantoscode
Copy link
Collaborator

I agree that inlining functions and classes can be harmful, but, if you have so many places where you don't want to inline functions, I think you're better off disabling reduce_vars altogether. This will affect inlining other constants and small things, but the compression gains from that are so small I just don't see how it's worth it.

Check out this section of the docs, or perhaps give it a try yourself. Disable reduce_vars or even compress entirely and see how it affects your bundle.

You see, I obsoleted reduce_funcs to make Terser have less bugs and be easier to maintain. These super specific options have diminishing returns in terms of byte loss and exponential increase in the complexity of this project.

@jareguo
Copy link
Author

jareguo commented May 18, 2020

I think you're better off disabling reduce_vars altogether. This will affect inlining other constants and small things, but the compression gains from that are so small I just don't see how it's worth it.

Hi. It's also about the performance. Once we disable reduce_vars, constant expressions will not be able to inlined. We are developing a game engine, which contains so many const numbers and math formulas. It's important for us to keep them inlined so we can use variables as many as we can without worrying about performance in runtime.

You see, I obsoleted reduce_funcs to make Terser have less bugs and be easier to maintain. These super specific options have diminishing returns in terms of byte loss and exponential increase in the complexity of this project.

I agree this. This issue is too trivial and our usage scenario is unusual. So if I were you I will probably close this issue. Every product has its trade off, it's OK.

@fabiosantoscode
Copy link
Collaborator

I see how this is not just about byte loss. I'll see what I can do. It might be possible to piggy-back on the implementation for /*#__NOINLINE__*/ to achieve this.

@marvinhagemeister
Copy link

marvinhagemeister commented Jul 15, 2020

We've just ran into this issue in the Preact team. We have some functions that should not be inlined and used reduce_funcs for that up until now. Not inlining those leads to smaller size output for us as the function calls itself recursively:

function w() {
  for (var n; (w.__r = u.length); )
     // ...
      n.some(function(n) {
        // ... bunch of code
            t != o &&
              (function n(l) {
                var u, i;
                if (null != (l = l.__) && null != l.__c) {
                  for (l.__e = l.__c.base = null, u = 0; u < l.__k.length; u++)
                    if (null != (i = l.__k[u]) && null != i.__e) {
                      l.__e = l.__c.base = i.__e;
                      break;
                    }
                  return n(l);
                }
              })(r)));
      });
}

EDIT: Related PR on the preactjs repo where we are trying to upgrade our build tooling preactjs/preact#2624

@make-github-pseudonymous-again

So here is the benchmark:

@jareguo I do not understand. Should you not include the definition of foo and Point inside the noninlined benches? Or maybe I misunderstand Terser. Is Terser inlining stuff that is also exported, or called in multiple places?

@fabiosantoscode
Copy link
Collaborator

This has been criminally overlooked, I really need to get on with fixing this.

@make-github-pseudonymous-again
Copy link

make-github-pseudonymous-again commented Mar 31, 2021

@fabiosantoscode I use terser through microbundle. I have the habit of exporting all defined functions. I assume that is why I had not run into inlining behaviour so far. I just did an experiment by removing exports of internal functions and indeed some inlining does happen. I understand this is done with bundle size in mind. However, the current implementation seems to inline the function definition without inlining the function call (there is a leftover surrounding closure). If this generated code is executed only once, no problem. However, when this is inside a hot code path it is bound to cause performance problems like the ones @jareguo reported.

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

No branches or pull requests

4 participants