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

Use JavaScript for code elimitation instead of templates #5417

Closed
nazar-pc opened this issue Jul 24, 2017 · 15 comments
Closed

Use JavaScript for code elimitation instead of templates #5417

nazar-pc opened this issue Jul 24, 2017 · 15 comments
Labels

Comments

@nazar-pc
Copy link
Contributor

The mix of plain JavaScript with templates breaks analysis and autocompletion in IDE and is generally hard to read and maintain.

Assuming Google Closure Compiler is always used for production builds (don't see the reason why it shouldn't) or any other relatively advanced tool like UglifyJS is used, we can put all of the code elimination work on those tools.

Let's turn this:

    var onerror = function() {
#if FETCH_DEBUG
      console.error('fetch: IndexedDB open failed.');
#endif
      Fetch.dbInstance = false;

#if USE_PTHREADS
      if (isMainThread) {
        if (typeof SharedArrayBuffer !== 'undefined') Fetch.initFetchWorker();
        removeRunDependency('library_fetch_init');
      }
#endif
    };

Into this:

    var onerror = function() {
      if (FETCH_DEBUG) {
        console.error('fetch: IndexedDB open failed.');
      }
      Fetch.dbInstance = false;

      if (USE_PTHREADS) {
        if (isMainThread) {
          if (typeof SharedArrayBuffer !== 'undefined') Fetch.initFetchWorker();
          removeRunDependency('library_fetch_init');
        }
      }
    };

Where FETCH_DEBUG and USE_PTHREADS definition can be prepended to existing JavaScript code.

When any of these variables are defined upfront and never change in runtime, Google Closure Compiler should be able to remove unnecessary branches entirely and open branches that should be kept.

Google Closure Compiler already does a very good job of deleting unused code, inlining short functions and so on, why not delegate the rest of work to it?
I think this might greatly improve readability and maintainability of the JavaScript part of this project.

@kripken
Copy link
Member

kripken commented Jul 24, 2017

I like this idea. It would take slightly more work, but not much: we don't run closure by default, because it's fairly slow, but what we could do is add code to the js optimizer (in the "shell cleanup" pass, probably) to match that pattern (i.e., if an if on a condition, where the condition is a global const). So basically our js cleanup code would be guaranteed to eliminate that code.

This would make -O0 builds larger (since they don't run closure or the js optimizer), but I think that's a reasonable tradeoff. Thoughts?

@nazar-pc
Copy link
Contributor Author

To be honest, I'm not sure it is worth investing an effort to write any code elimination in-house. It will be potentially error-prone and besides that in most projects developers will either use Closure Compiler option on build time or will add some post-processing to the file afterwards. I'd just give those tools a chance to do what they do well (code minification) and focus Emscripten on code compilation part.

In other words, use cases where code elimination is desired and no other tool is run on top of the JavaScript build are fairly limited.

@curiousdannii
Copy link
Contributor

curiousdannii commented Jul 25, 2017

How would you handle library_async.js's #if ASYNCIFY and #if EMTERPRETIFY_ASYNC? If the answer is simply to have two separate object literals, how would you handle my async library where only a handful of functions are conditional? I could merge multiple objects together but that would get messy fast.

Edit: I guess it would be possible to move the conditionals inside the functions, but what about the __deps? Is the library processing code smart enough to handle something like this:

glem_fileref_create_by_prompt__deps: [ EMTERPRETIFY_ASYNC ? '$EmterpreterAsync' : 'emscripten_async_resume', 'fileref_to_id'],

Edit: after thinking more, I'm pretty sure it would be smart enough, as fairly arbitrary code is allowed.

@nazar-pc
Copy link
Contributor Author

@curiousdannii, I do not entirely understand your concerns. If there are multiple parts of object literal then yes, we'll need to split it into pieces if we want to use plain JavaScript, nothing special about it.

Mentioned minifiers operate with AST (as far as I remember) and ternary operator for them is no more difficult than regular if/else.

You can play with Closure Compiler here: https://closure-compiler.appspot.com/ (do not forget to wrap variables into self-calling function so that they will not be in global scope.

@curiousdannii
Copy link
Contributor

curiousdannii commented Jul 25, 2017

@nazar-pc This has nothing to do with minification. The .js library files are processed by emcc, not output directly, and so you need to be careful that the conditional preprocessing still works correctly. I think it probably will work fine. But I think it will make it very messy.

Perhaps the best option would be to allow for both: use JS constant if statements for most things, but allow preprocessor directives when it would otherwise be too messy.

@nazar-pc
Copy link
Contributor Author

I think we can start with small things first and see how it goes. If it gets too messy with JS constants I have nothing against using more appropriate tools. The whole point of proposal is to make things easier to work with, not harder.

@kripken
Copy link
Member

kripken commented Jul 25, 2017

@curiousdannii, I think you're right, this idea could work well for complete lines of code (it's just adding a condition around that code) but not for object literals and perhaps other things (like a fragment of a line of code). So I don't think it's practical to expect to be able to avoid proprocessing entirely.

nazar-pc referenced this issue Nov 13, 2017
…y(), emscripten_enter_soft_fullscreen() and emscripten_exit_soft_fullscreen() which allow a more intelligent switch between fullscreen modes with different preset display modes. The original emscripten_request_fullscreen() is still available, and should be used in scenarios where it's desired that the html5.h layer should perform no changes whatsoever to the DOM tree.
@juj
Copy link
Collaborator

juj commented Nov 13, 2017

This does not look like a good direction. It is not ideal to rely on dynamic code analysis to optimize something that is already optimizable statically. This would necessitate using Closure to get to good lean sized result, and there are developers who don't want to use Closure due to not having to deal with Java (as it stands right now), or not having to deal with defining minified-nonminifed boundaries for inferfacing with external code, or not wanting to add an extra costly compilation step, or because they do want to support asm.js, or due to wanting to use some other minifier, which might not be as sophisticated to do this particular optimization.

I don't agree that #if USE_PTHREADS would be particularly harder to read than if (USE_PTHREADS), that comes down to subjective preferences rather than anything else.

It should also be noted that this applies to a fairly internally developed set of runtime, users can go whichever way they choose in their own library_xx.js files.

@nazar-pc
Copy link
Contributor Author

First of all, static optimizations require an effort to do them and as we can see, modern Closure Compiler shows that not everything is actually statically optimized even on the very high level. I do not see why should we spend any time on manually optimizing something that can be completely automated and gain way better results than it is possible to achieve manually with reasonable time spent on it.

Java use is important, hopefully in nearest months it will be possible to switch to JavaScript version of Closure Compiler, so this concern will be always valid. However, emsdk includes it out of the box, so it is not that big of a deal.

Also I'd go as far as saying that Closure Compiler should be enabled by default and non-optimized builds should only be used by Emscripten developers for debugging. This will require to eliminate all of the eval() calls and some other stuff, which I've already started working on. So for external developers there should be only 1 option: Closure Compiler enabled by default and they shouldn't see any practical difference comparing to current output.

I don't agree that #if USE_PTHREADS would be particularly harder to read than if (USE_PTHREADS), that comes down to subjective preferences rather than anything else.

No, it comes down to tooling. Regular JavaScript can be analyzed by lots of existing tools, preventing multitude of typos and other kinds of errors. While currently powerful IDE like WebStorm struggles to provide basic syntax highlighting when using #if USE_PTHREADS and such. Also these templates are mostly on the same level of indentation, which makes reading even harder. There are places where templates are useful, but in long term that should be an exception, not a rule (IMHO, of course).

@dschuff
Copy link
Member

dschuff commented Nov 14, 2017

I'm sympathetic to the goal of making the code more tooling-friendly. However one problem with relying on optimization (as opposed to making it explicit) is that it's much less clear exactly what is happening and where (i.e. which optimizations are being applied) and easy to lose the optimizations without noticing (which in this case would result in bloat in the output). If we replace the existing ifdef-based way of excluding unused library code, we should have some testing to ensure we don't regress.

Also if we do this now, we'd basically make Java a hard dependency for emscripten users and I'm not sure I want to do that.

@dschuff
Copy link
Member

dschuff commented Nov 14, 2017

I guess one other argument against requiring closure is that it reduces debugability for unoptimized builds (i.e. you either don't run closure, in which case you get all the syscalls you don't need, or you run it anyway and obfuscate everything). Can it be run in such a way that it only does the dead-code elimination that we want?

@nazar-pc
Copy link
Contributor Author

Think about it as about C/C++ code compilation: how often do you try to beat compiler with manual optimizations? My guess would be not that often, especially considering that compiler often does a better job at optimizations.

Can it be run in such a way that it only does the dead-code elimination that we want?

  1. I don't think so
  2. There is a high chance that even those who don't use Closure might have some other tool like UglifyJS in their process, and those tools should be able to do dead code elimination just fine too
  3. Potentially we can use UglifyJS or something similar for this purpose in Emcripten with everything else, including names mangling, turned off (AFAIK, UglifyJS supports such mode of operation) as the first pass of optimizations

Now I think 3 is particularly interesting option to consider.

@curiousdannii
Copy link
Contributor

curiousdannii commented Nov 14, 2017

The UglifyJS devs are... less than ideal. They refuse to make a changelog. I don't think it would be a safe dependency for a project like this.

Babel-minify is good, but could be considerably slower. I haven't tested though. It also changes asm.js code so you couldn't just apply to the whole output.

Maybe Closure is the best option. I just don't use it because I don't like ADVANCED_OPTIMIZATIONS. It makes writing custom library files, or using pre/post-js, trickier. Maybe SIMPLE_OPTIMIZATIONS could be the default instead (with an option to switch to ADVANCED_OPTIMIZATIONS)? How much bigger would that make the files?

Can it be run in such a way that it only does the dead-code elimination that we want?

Are you saying that you want DCE but not other minification? Maybe babel-plugin-minify-dead-code-elimination would be an option in that case.

@nazar-pc
Copy link
Contributor Author

How much bigger would that make the files?

No way to tell without testing.

Are you saying that you want DCE but not other minification? Maybe babel-plugin-minify-dead-code-elimination would be an option in that case.

Not necessarily want, but I think it is an option to consider just so that we don't force everyone to use Closure Compiler in ADVANCED mode and don't need to manually optimize everything either. We'll need to explore available tools if it sounds like a good idea.

@stale
Copy link

stale bot commented Sep 19, 2019

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Sep 19, 2019
@stale stale bot closed this as completed Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants