google / trace-viewer

https://github.com/catapult-project/catapult/tree/master/tracing#readme
487 stars 84 forks source link

Consider using rjsmin #880

Closed natduca closed 9 years ago

natduca commented 9 years ago

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/build/scripts/rjsmin.py&q=rjsmin&sq=package:chromium&type=cshttps://codereview.chromium.org/252633006&l=22

natduca commented 9 years ago

@ChrisCraik @dj2 @anniesullie fyi

anniesullie commented 9 years ago

Looks like we'd want to hook rjsmin into https://github.com/google/trace-viewer/blob/master/third_party/tvcm/tvcm/generate.py ?

Since tvcm is in the third_party directory, is it from a separate repo? If so, where does the repo live?

Looks like rjsmin can be pulled from here: http://opensource.perlig.de/rjsmin/

randomascii commented 9 years ago

I've found it useful to debug traceviewer live on shipping versions of Chrome. Presumably rjsmin would make this scenario more difficult. How much do we gain by doing this? I assume that rjsmin would reduce the Chrome download size?

dj2 commented 9 years ago

Trace viewer is a non-trivial amount of the total GRD when added into chrome. Compressing it should help with that.

We'd probably also want to strip comments, extraneous white space, etc, if possible.

randomascii commented 9 years ago

Okay, as long as the tradeoff is worth it (i.e.; enough savings to make it worthwhile)

nedn commented 9 years ago

As long as we provide a build flag that enable turning minifying trace viewer on & off, I think we should be ok? Also why do we want to use this over closure's jscompiler?

dj2 commented 9 years ago

This is in python which we can run on the bots. Closure isn't, so we can't run it on the bots.

I'm not sure if we need to the flag to turn minification on/off, I'm not sure how you'd even use it as this is called through the chrome build system.

nedn commented 9 years ago

I am not sure about the bot policy but does https://pypi.python.org/pypi/closure/20140110 help?

dj2 commented 9 years ago

That seems more complicated then just running one done in python. What benefit would closure give us?

nedn commented 9 years ago

I suspect closure can do a better job at minifying javascript since it's supported & used by a large community.

anniesullie commented 9 years ago

Update on this:

1) I ran generate_about_tracing with rjsmin, closure, and no minifier. Got the following:

Unminified: 189592 about_tracing.html 1757785 about_tracing.js

rJSmin: 138269 about_tracing.html 1272878 about_tracing.js

closure: 138269 about_tracing.html 1032984 about_tracing.js

So Ned is correct that closure can do a better job at minifying JavaScript. I doubt we are going to find a minifier that does not use node or rhino which is on par with the ones which do. rJSmin is based on Douglas Crockford's jsmin from 2003. It just runs regular expressions to remove comments and whitespace. Still, it minifies quite well.

To Bruce's point that it's hard to debug minified JS, I think rJSmin actually gives us an advantage here. Since it's not smart enough to rename local variables, shuffle code, etc, it should work quite well with the "{}" button in devtools which formats JavaScript. Of course there will be no comments.

What is the best way to test my rjsmin-minified files?

natduca commented 9 years ago

Wow! this is neat!

rjsmin seems extremely promising. Thats a serious size savings.

As far as testing, I think the only real easy way is to try a build with this minification turned on. You might be able to run the tests somehow by vulcanizing the test files together but itd require lots of hacks.. We could discuss offline whether its necessary to do that, and I could give you some pointers if its useful.

As far as debugging goes, we try to rarely debug about:Tracing embedded in chrome. The purpose of the devserver is specifically so that we can do whatever we want with the outpt code. :)

anniesullie commented 9 years ago

This is all landed.

One note: When I built the ToT trace-viewer into chromium, the record button threw a JS error. This also happens WITHOUT any of my minification patches, so I believe it's unrelated to my changes. I'm headed out on vacation and don't have time to bisect the cause, unfortunately. Viewing a trace works correctly with minification.

For debugging, since there is no renaming going on, you can hit the "{}" button in devtools and get decently formatted code, but it's missing comments.

nedn commented 9 years ago

I added the step of vulcanizing the trace-viewer & reporting size in travis build so we can keep track of size changes per submit.

However, since Annies' fix, size is still pretty large:

4.80s$ ./vulcanize_trace_viewer --config=chrome

The command "./vulcanize_trace_viewer --config=chrome" exited with 0.

4.72s$ ./vulcanize_trace_viewer --config=full

The command "./vulcanize_trace_viewer --config=full" exited with 0.

0.01s$ stat -c%s bin/trace_viewer_chrome.html

1357449

The command "stat -c%s bin/trace_viewer_chrome.html" exited with 0.

0.01s$ stat -c%s bin/trace_viewer_full.html

1357454

(https://travis-ci.org/google/trace-viewer/builds/57127376)

natduca commented 9 years ago

Not clear why this should be reopened. We're using Rjsmin, so this should probably get closed.

If we want another new bug about size of trace viewer, thats fine... I think its good having a discussion about how to be smaller but also alternatives like how to deal with size. Lots of options here. But better on a fresh thread.