search

dojo / util

Dojo 1 - build utilities. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
60 stars 105 forks source link

fixes #17105, Prevent 60s closure compiler timeout #26

Closed wkeese closed 9 years ago

wkeese commented 9 years ago

https://bugs.dojotoolkit.org/ticket/17105

https://github.com/google/closure-compiler/blob/v20120917/src/com/google/javascript/jscomp/Compiler.java#L194

Use Java reflection to call ExecutorService.shutdown() on Compiler's compilerExecutor field when we are finished compiling JS.

This prevents the default 60 seconds timeout that the ExecutorService will wait before stopping Threads that have no work to do.

WARNING - This code uses the private compilerExecutor field of Compiler, and so may break if Compiler's internal implementation changes.

wkeese commented 9 years ago

@gitgrimbo, I'll push this if no one else does, but I'm having trouble running builds at all now. When I do:

$ ./build.sh --profile profiles/base.profile.js --release

it just gives me a bunch of errors (with or without your patch), and AFAICT doesn't generate any output.

What command should I be using for a spot check of this?

gitgrimbo commented 9 years ago

That seems fine to me. What project are you building? FYI I've been using the following (action: 'release' is in my profile, hence missing from the command line):

node src/dojo/dojo.js load=build --profile profiles/app.profile.js optimize= layerOptimize=

I'm going to push a sample project soon - the intention is to have a consistent basis for performance testing my builds.

wkeese commented 9 years ago

That seems fine to me.

Yeah, there must be some environment problem, or maybe it's an old profile or something. I don't know [yet].

What project are you building?

As I said above, profiles/base.profile.js. Looking now I see that doesn't do much of anything. But I also tried mobile.profile.js:

$ ./build.sh --release --profile profiles/mobile.profile.js

which leads to the same result (for me).

PS: Both these profiles are just for the dojo code itself; they don't bundle in any application code. Also, I didn't know that you could add an action to the profile; that's cool. I see it in demos-all.profile.js too.

wkeese commented 9 years ago

Ah OK, when @bryanforbes checked in code in dojo to use Intern it broke the build process, because the builder gets confused by all the code in the new node_modules directory. Of course, this is unrelated to your patch. I filed https://bugs.dojotoolkit.org/ticket/18648.

wkeese commented 9 years ago

Now my problem is that I can't reproduce that 60s hang without your fix. I know I've seen it in the past though, but here's my current output. I added the flag to make sure that I'm using node:

mac:buildscripts bill$ git show HEAD
commit ec58d6c06fd8373b8b3f57980671393db8ecfbcb
Author: dylans <dylan@dojotoolkit.org>
Date:   Sun Jul 12 07:28:02 2015 -0700

    refs #18640, add repository field to package.json to make npm not complain

...

mac:buildscripts bill$ ./build.sh --profile profiles/mobile.profile.js --release --bin node
processing profile resource /ws/trunk/util/buildscripts/profiles/mobile.profile.js
info(107) Package Version: package: dijit; version: 1.11.0-pre
processing profile resource /ws/trunk/dijit/dijit.profile.js
info(107) Package Version: package: dojox; version: 1.11.0-pre
processing profile resource /ws/trunk/dojox/dojox.profile.js
info(107) Package Version: package: dojo; version: 1.11.0-pre
processing profile resource /ws/trunk/dojo/dojo.profile.js
discovering resources...
starting reading resources...
starting processing raw resource content...
starting tokenizing resource...
starting processing resource tokens...
starting parsing resource...
starting processing resource AST...
warn(224) A plugin dependency was encountered but there was no build-time plugin resolver. module: dijit/Fieldset; plugin: dojo/query
...
warn(224) A plugin dependency was encountered but there was no build-time plugin resolver. module: dojo/request/registry; plugin: dojo/request/default
warn(216) dojo/has plugin resource could not be resolved during build-time. plugin resource id: host-browser?dom-addeventlistener?:../on:; reference module id: dojo/request/watch
starting executing global optimizations...
starting writing resources...
starting cleaning up...
waiting for the optimizer runner to finish...
starting reporting...
Report written to /ws/trunk/release/dojo/build-report.txt
Process finished normally.
    errors: 0
    warnings: 61
    build time: 7.318 seconds
mac:buildscripts bill$

I get the same result (ie about 7s) when testing against 1.10.0.

gitgrimbo commented 9 years ago

7 seconds sounds quick (but I've been building an app using dojo, dijit and dojox). What optimisation settings are being used?

wkeese commented 9 years ago

You could try it yourself to see, but here's a more precise command:

$ ./build.sh action=clean,release layerOptimize=closure stripConsole=all cssOptimize=comments profile=profiles/mobile.profile.js

How long does that command take for you with and without the patch? Is there a difference?

gitgrimbo commented 9 years ago

Hi, on my laptop (aggressive anti-virus, Win7, Intel64 Family 6 Model 42 Stepping 7 GenuineIntel ~2501 Mhz, 8GB RAM) I get these times:

@1.10.4

Process finished normally.
        errors: 0
        warnings: 62
        build time: 109.422 seconds

@fix17105-closure-executor-service-shutdown (as above, minus ~60s closure shutdown)

Process finished normally.
        errors: 0
        warnings: 62
        build time: 52.914 seconds

@x-use-nodefs-for-copy-transform (https://github.com/gitgrimbo/util/tree/x-use-nodefs-for-copy-transform, add useFsCopy=1 at command line)

Process finished normally.
        errors: 0
        warnings: 62
        build time: 10.846 seconds

If you're using a Mac, perhaps that handles the Node-to-Java process(es) differently to Windows?

wkeese commented 9 years ago

Are those results from the exact command I typed in https://github.com/dojo/util/pull/26#issuecomment-123617151?

gitgrimbo commented 9 years ago

Yep, run from Git Bash within the util/buildscripts folder of a checked out repo.

$ ./build.sh action=clean,release layerOptimize=closure stripConsole=all cssOptimize=comments useFsCopy=0 profile=profiles/mobile.profile.js

wkeese commented 9 years ago

OK, I reproduce now (and confirm the fix too)! As you guessed, the problem only happens on Windows. Anyway, I will push your fix now. Thanks for all the work on this!

wkeese commented 9 years ago

OK, pushed as 7085ef15f71a4dbd7b8662044d7155f185dc60c9.

gitgrimbo commented 9 years ago

Thanks. Could you also take a look at https://github.com/gitgrimbo/util/tree/x-use-nodefs-for-copy-transform.

The following gist shows times for using Node's fs copy instead of existing cmd /c copy approach for the copy transform.

https://gist.github.com/gitgrimbo/950f3301ce7b9d8ce18f.

Long story short, 11 seconds down to 4 seconds on my PC, and 45 seconds down to 7 seconds on my laptop.

I haven't tested using the Java/Rhino runner yet.

I can submit a PR if preferred.