gopherjs icon indicating copy to clipboard operation
gopherjs copied to clipboard

Optional aggressive dead code elimination

Open neelance opened this issue 11 years ago • 33 comments

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 avatar Feb 25 '15 13:02 neelance

Related to #136

neelance avatar Feb 25 '15 13:02 neelance

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.)

dmitshur avatar Feb 26 '15 04:02 dmitshur

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

benechols-wf avatar Feb 26 '15 19:02 benechols-wf

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

neelance avatar Feb 26 '15 19:02 neelance

Then I say this sounds awesome.

benechols-wf avatar Feb 26 '15 22:02 benechols-wf

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!

albrow avatar Mar 04 '15 20:03 albrow

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.

stmichaelis avatar Mar 18 '15 19:03 stmichaelis

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.)

skybrian avatar May 08 '15 16:05 skybrian

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.

influx6 avatar Oct 13 '15 19:10 influx6

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.

vron avatar Nov 03 '15 23:11 vron

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.

dmitshur avatar Nov 04 '15 07:11 dmitshur

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.

alexandrestein avatar Nov 04 '15 09:11 alexandrestein

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.

niklaskarla avatar Nov 07 '15 09:11 niklaskarla

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.

flimzy avatar Nov 07 '15 10:11 flimzy

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

neelance avatar Nov 07 '15 12:11 neelance

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).

vron avatar Nov 07 '15 22:11 vron

@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.

flimzy avatar Nov 08 '15 12:11 flimzy

@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.

mpl avatar Jun 02 '16 01:06 mpl

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.

theclapp avatar Jun 02 '16 02:06 theclapp

@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.

paralin avatar Sep 21 '16 18:09 paralin

@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?

myitcv avatar Dec 12 '16 18:12 myitcv

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

dominikh avatar Dec 14 '16 06:12 dominikh

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

tj avatar Feb 16 '17 17:02 tj

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

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

myitcv avatar Mar 29 '17 15:03 myitcv

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

neelance avatar Mar 29 '17 16:03 neelance

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 avatar Dec 20 '17 22:12 glycerine

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

glycerine avatar Dec 23 '17 17:12 glycerine

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.

ColtonProvias avatar Mar 04 '18 06:03 ColtonProvias

Reflection is an anti-pattern.

shelby3 avatar May 20 '18 17:05 shelby3

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

md9projeto avatar Jul 09 '18 19:07 md9projeto