Skip to content

Optional aggressive dead code elimination #186

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

Open
neelance opened this issue Feb 25, 2015 · 33 comments
Open

Optional aggressive dead code elimination #186

neelance opened this issue Feb 25, 2015 · 33 comments
Labels
NeedsHelp Community contributions are welcome for this feature! Proposal Accepted Proposal

Comments

@neelance
Copy link
Member

Proposal:
A package-level flag in the source code that can only be used in the main package which switches the dead code elimination into "aggressive" mode. In this mode, the reflection can only invoke methods that are also referenced somewhere else in the code. If they are not referenced, the DCE eliminates them at compile time and calling them via reflection will cause a panic properly describing the situation.

This flag is an opt-in feature to produce smaller output. Since it can only be set in the main package, it is the application author's responsibility to make sure that it does not break the program at runtime. The author can always add a dummy reference somewhere to make the DCE include a certain method.

@neelance
Copy link
Member Author

Related to #136

@dmitshur
Copy link
Member

dmitshur commented Feb 26, 2015

I don't have very good feedback, but I'll share it anyway (since no one else has).

My stance towards this is neutral. I'm not against it, as it won't negatively impact things (except increase project complexity and take up your developer time a little, as any feature does). But I'm not sure if it's something that will provide lots of value (it may, but I'm just not sure at this time).
Edit on 2016-05-09: This is no longer up to date, my stance is significantly in favor of this by now. See my comment below.

For reference, I am extremely excited about work on the assume-blocking branch because I think that will be very very beneficial!
Edit on 2016-05-09: This has been long done, and this was an incredible improvement!

(Keep in mind this feedback is coming from 1 person and it reflects my personal uses/gripes about GopherJS, it's possible others will feel very differently about this issue. For my current needs, the generated code size is acceptable.)

@benechols-wf
Copy link

What advantage will this provide vs existing javascript tree shaking tools?

@neelance
Copy link
Member Author

You mean javascript optimizers? They can't do that kind of DCE, it's too hard to analyse.

@benechols-wf
Copy link

Then I say this sounds awesome.

@albrow
Copy link
Contributor

albrow commented Mar 4, 2015

I'm definitely in favor of this. I'm planning to serve gopherjs-generated to clients and anything that reduces the size of the resulting js would be a godsend!

@stmichaelis
Copy link

Even if a bit late for this discussion: Thumbs up for this feature, really beneficial for frontend code. Should be worth the additional effort for the developer checking for all needed references.

@neelance neelance modified the milestone: 1.0 Mar 26, 2015
@skybrian
Copy link

skybrian commented May 8, 2015

I haven't used GopherJS yet, but this is an important feature in other environments like GWT and Dart. It might be a good idea to think about how to conveniently annotate certain types as requiring reflection. (For example, types that are serialized via JSON.)

@influx6
Copy link

influx6 commented Oct 13, 2015

I definitely would love something like this, as of right now, am using reflect.Kind to do type matching only but I am aware the issue with the reflect package and how goperherjs can't yet fully indiscriminately remove it due to its bags of tricks. Would be nice to be able to specify or at least have means of shaking of unnecessary extra bits. As of right now, I tend to have binaries entering the 6MB range and above, compression is awesome but still one of the core things that makes JS sell is the fact that we can in some level control the sizes of the packages, plus It would not sound well to tell someone that their JS files are in those ranges or how nice it would be since the browser can do GZIP decompression already.

You guys have done awesome work and this would be a great addition to the table and would really gear the uptake of go fully to produce JS libraries.

@vron
Copy link

vron commented Nov 3, 2015

I would also love something like this, I am seeing binary size of 8Mb+ (before compression) due to pull-in of a multitude of standard library packages of which only small parts are used.

It would also be amazing if it was a possibility to "profile" how large part of the binary size comes from different packages.

@dmitshur
Copy link
Member

dmitshur commented Nov 4, 2015

My previous stance on this was neutral. However, by now, I'd like to upgrade it to "I'd really like this feature too."

I'm primarily noticing large sizes (5-6 MB uncompressed) when importing net/http package and using it directly, instead of going through the lower level XHR API. (And it will only get worse once HTTP 2.0 support is added to it.) That leads to slower page loads (1+ second) just because parsing that much JS is relatively slow.

It would also be amazing if it was a possibility to "profile" how large part of the binary size comes from different packages.

It's quite easy to get a good idea by looking at unminified JS output. It's quite readable, and you can see the relative size of each package being included.

@alexandrestein
Copy link

This feature would be great.
For web application the needed time to parse the JS file is quite long because of its pretty big size I guess.

Remove dead code should speed up the transmissions and the parsing.

@niklaskarla
Copy link

I would really like to see this feature implemented. I am also experiencing huge binaries, almost 10Mb, mainly due to compiled standard libraries, of which I only use a fraction of the functionality.

@flimzy
Copy link
Member

flimzy commented Nov 7, 2015

I just want to mention, for those who are struggling with the compiled JS sizes, I've had good experience with external JS minifiers at reducing the output of my GopherJS compile. This doesn't obsolete such a feature request--having GopherJS do its own DCE is, of course, ideal.

Some sample data, done with uglifyjs2:

Original output from GopherJS:  5747663
uglifyjs with no options:       4230579 (26.4% reduction)
uglifyjs -m                     3136781 (45.4% reduction)
uglifyjs -m -c                  3023747 (47.4% reduction)

But some caution should be used, especially with the more aggressive features of a minifier.

In particular, the -c option (which enables uglifyjs's own DCE) may be dangerous with GopherJS output, based on something I read long ago, and can't find at the moment. I expect @neelance comment on this more authoritatively.

As far as I understand, the other options should be safe. Uglifyjs without any options basically just removes whitespace, which ought to be safe. The -m options mangles variable names to be shorter, and I expect is safe.

In any case, I have yet to experience problems using uglifyjs (even with the -c option), but as always, YMMV.

@neelance
Copy link
Member Author

neelance commented Nov 7, 2015

@flimzy Are your numbers for GopherJS with its minification enabled (-m flag)?

@vron
Copy link

vron commented Nov 7, 2015

I have investigate a bit more and wanted to share some further thoughts. Initially I had my gopherjs generated script included in the head tag, but due to to long download time I haved moved it to the end of body to be able to show the initial gui quicker. That works fine as both download and parsing is done in a separate thread.

However, even on my laptop (phone is much slower) it takes 800ms just to run the closure wrapping all the generated code. This means that the entire gui blocks for 1s+ while running this closure.

This time will of course decrease by eliminating the dead code, but would most likely still be clearly visible when e.g. typing in an input box and no events are handled for 100s of ms.

Have anyone else experienced this? One possible solution could be to not have gopherjs wrap all the generated code in one anonymous function, but do it for each package individually and instead pass a scope variable as an argument. That would allow for yielding to the event loop between each package initiation/evaluation (eg either a settimeout or load the generated code as several script files).

@flimzy
Copy link
Member

flimzy commented Nov 8, 2015

@neelance, that's a great question. And looking over the .js file I used, it looks like it was actually a bundle of some GopherJS output and some other node modules. So I've redone the test completely, this time both with gopherjs build and gopherjs build -m. Results below:

                                    Size    +/- gopherjs -m
                                    ------- ---------------
gopherjs build                      4645502 +54.7%
gopherjs build, uglifyjs            3597995 +19.8%
gopherjs build, uglifyjs -m         2676827 -10.9%
gopherjs build, uglifyjs -m -c      2580348 -14.1%
gopherjs build -m                   3003427 --
gopherjs build -m, uglifyjs         2949174 -1.8%
gopherjs build -m, uglifyjs -m      2614524 -12.9%
gopherjs build -m, uglifyjs -m -c   2518013 -16.2%

So clearly the greatest gains are had when using gopherjs -m, but uglifyjs can still squeeze more size optimization out of the file.

@mpl
Copy link

mpl commented Jun 2, 2016

@neelance Is there anything I can do to help on that front? or with any other way to minify some more? We'd love to use gopherjs in camlistore but it looks like the output size might end up being the show stopper for us.

@theclapp
Copy link

theclapp commented Jun 2, 2016

GopherJS downloads are so large because they include all the JS that implements all the Go standard libraries, yes? (Or at least whatever's imported in the current application.) Couldn't that at least be pre-compiled and given as a static download, and then the only thing that changes from page to page is the application code? The browser would still have to parse it all, but it seems like it could help with caching.

Sorry if this has been shot down before.

@paralin
Copy link
Contributor

paralin commented Sep 21, 2016

@theclapp See #524 - could be a solution. If something like Webpack could understand the code structure, it could bundle the Go standard libs into a vendor.js file.

@myitcv
Copy link
Member

myitcv commented Dec 12, 2016

@neelance do you have a prototype of this aggressive dead code elimination?

Otherwise I wonder @dominikh whether it would be possible to adapt https://github.com/dominikh/go-unused to rewrite a copy of a target package (and all its transitive dependencies, including the standard library) to eliminate unused code prior to running the gopherjs build "phase"? Might not end up being the final implementation as far as this issue is concerned but could be a quick means of trying out this approach?

@dominikh
Copy link
Member

It would probably be easier to prototype this in GopherJS itself than to make go-unused do it.

@tj
Copy link

tj commented Feb 16, 2017

I'm noticing it's really easy to accidentally bloat your client code. I have a few shared package with build tags, but then I'm pulling in all the server dependencies as well. Definitely looking forward to DCE :D

@myitcv
Copy link
Member

myitcv commented Mar 29, 2017

@neelance / @shurcooL / anyone else currently working on this?

If not, I'm tempted to start hacking/planning what it would look like...

@neelance
Copy link
Member Author

I currently don't have the time available to work on it. Feel free to investigate.

@glycerine
Copy link

glycerine commented Dec 20, 2017

A couple of brainstorms:

a) runtime behavior observation

A super simple runtime trace approach: have a compiler flag -in-use-profile where, for each function written, there is an accompanying global "InUse" bool var generated too. In Go this might look like:

var InUseFuncName bool // added because `in-use-profile` flag was on

func FuncName() {
    InUseFuncName = true // added because `in-use-profile` flag was on
    ... rest of the usual body...
}

Then at program conclusion (or by polling at intervals) save the set of not InUse functions. This would probably have to be filtered for false negatives, but false positives would be impossible (a function marked in use is definitely in use and should not be discarded). Then consider deleting functions not in the InUse set.

b) guru based caller/callee data, from static Go analysis

https://github.com/golang/tools/blob/master/cmd/guru/callers.go#L19

In particular, guru probably has some really good hints about possible calls through interfaces.

@glycerine
Copy link

possibly useful, rollup.js impements tree shaking.
https://medium.com/@Rich_Harris/tree-shaking-versus-dead-code-elimination-d3765df85c80

@ColtonProvias
Copy link

ColtonProvias commented Mar 4, 2018

Tree-shaking may be the better method of going about this. I haven't worked with parsing the AST, but wouldn't a tri-color mark-and-sweep algorithm be pretty much exactly what we want for this? In my opinion, that may be the first thing that should be tackled before we start into further optimizations like function inlining, checking of conditions for dead blocks, etc.

Right now we already have everything extending off of main, so we already have a place to gather the roots from.

@glycerine In regards to rollup, I have used it before. Rollup primarily removes functions that aren't explicitly imported by analyzing the import graph. It's a little limited due to the dynamic nature of JavaScript. JavaScript is pretty much a case most things being the equivalent of map[string]interface{}, so removing aggressively would be dangerous. With Go being a little bit more static than JavaScript, we can afford to be more aggressive in our dead code elimination.

@shelby3
Copy link

shelby3 commented May 20, 2018

Reflection is an anti-pattern.

@md9projeto
Copy link

md9projeto commented Jul 9, 2018

I hope this thread helps,It describes how Dart do three Shaking, wich in fact is tree growing.
https://groups.google.com/a/dartlang.org/forum/#!topic/compiler-dev/KI2JFRsIGCc

@flimzy
Copy link
Member

flimzy commented Jul 10, 2018

@md9projeto This was already mentioned above. See #259 :)

@benma
Copy link
Contributor

benma commented Aug 16, 2022

In my humble opinion, this issue should be the top priority for this project (it seemed to be in 2015 😄). The output sizes are bad, and solving this issue could go a really long way in mitigating the problem.

@neelance are you still interested in pursuing this? Or @nevkontakte?

@nevkontakte
Copy link
Member

@benma to be completely honest with you, I barely have enough time to keep up with new Go releases and major language features (embed or generics, for example). Whenever I come across low-hanging fruits that can reduce generated code size, I do them, but this change is not an easy one. Besides that I have to worry about general code health and maintenance of the project, so I end up stretched pretty thin.

If you would like to tackle this feature request, your contribution will be very welcome, and I'll do what I can to help you understand the codebase and weed out bugs. If not, eventually me or @flimzy might get to it, but we can't make promises on when.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsHelp Community contributions are welcome for this feature! Proposal Accepted Proposal
Projects
None yet
Development

No branches or pull requests