-
Notifications
You must be signed in to change notification settings - Fork 570
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
Comments
Related to #136 |
I don't have very good feedback, but I'll share it anyway (since no one else has).
For reference, I am extremely excited about work on the (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.) |
What advantage will this provide vs existing javascript tree shaking tools? |
You mean javascript optimizers? They can't do that kind of DCE, it's too hard to analyse. |
Then I say this sounds awesome. |
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! |
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. |
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.) |
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. |
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. |
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
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. |
This feature would be great. Remove dead code should speed up the transmissions and the parsing. |
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. |
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:
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. |
@flimzy Are your numbers for GopherJS with its minification enabled ( |
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). |
@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
So clearly the greatest gains are had when using |
@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. |
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. |
@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? |
It would probably be easier to prototype this in GopherJS itself than to make go-unused do it. |
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 |
I currently don't have the time available to work on it. Feel free to investigate. |
A couple of brainstorms: a) runtime behavior observation A super simple runtime trace approach: have a compiler flag
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. |
possibly useful, rollup.js impements tree shaking. |
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 |
Reflection is an anti-pattern. |
I hope this thread helps,It describes how Dart do three Shaking, wich in fact is tree growing. |
@md9projeto This was already mentioned above. See #259 :) |
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? |
@benma to be completely honest with you, I barely have enough time to keep up with new Go releases and major language features ( 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. |
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.The text was updated successfully, but these errors were encountered: