You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Henry Saputra <hs...@apache.org> on 2012/09/15 01:32:51 UTC
Review Request: Disabled thread in the closure compiler code and add thread
management in the Shindig ClosureJSCompiler class
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/
-----------------------------------------------------------
Review request for shindig and Dan Dumont.
Description
-------
Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
Diffs
-----
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
Diff: https://reviews.apache.org/r/7122/diff/
Testing
-------
Run smoke test with common container with debug sets to false to run the closure js compiler.
Thanks,
Henry Saputra
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11635
-----------------------------------------------------------
Ship it!
Ship It!
- Stanton Sievers
On Sept. 17, 2012, 5:32 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2012, 5:32 p.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disable thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/
-----------------------------------------------------------
(Updated Sept. 17, 2012, 11:13 p.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
Summary (updated)
-----------------
Disable thread in the closure compiler code and add thread management in the Shindig ClosureJSCompiler class
Description
-------
Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
Diffs
-----
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
Diff: https://reviews.apache.org/r/7122/diff/
Testing
-------
Run smoke test with common container with debug sets to false to run the closure js compiler.
Thanks,
Henry Saputra
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/
-----------------------------------------------------------
(Updated Sept. 17, 2012, 5:32 p.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
Changes
-------
Address Stanton review to make the stack size configurable
Description
-------
Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
Diffs (updated)
-----
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
Diff: https://reviews.apache.org/r/7122/diff/
Testing
-------
Run smoke test with common container with debug sets to false to run the closure js compiler.
Thanks,
Henry Saputra
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11612
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Dumont
On Sept. 15, 2012, 4:58 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2012, 4:58 p.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/
-----------------------------------------------------------
(Updated Sept. 15, 2012, 4:58 p.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
Changes
-------
adding Stanton as reviewer
Description
-------
Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
Diffs
-----
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
Diff: https://reviews.apache.org/r/7122/diff/
Testing
-------
Run smoke test with common container with debug sets to false to run the closure js compiler.
Thanks,
Henry Saputra
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
> On Sept. 15, 2012, 2:52 p.m., Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java, line 263
> > <https://reviews.apache.org/r/7122/diff/1/?file=155595#file155595line263>
> >
> > The javadoc for this method states "Disable threads. This is for clients that run on AppEngine and don't have threads." That concerns me, as this is certainly not the case we're in. Is the javadoc incomplete?
>
> Henry Saputra wrote:
> Its kinda misleading bc internally the closure JS compiler doe not use thread pool. So when compiling large number of JS resources it could spawn a lot of threads.
>
> I am planning to contribute code to add thread pool support to the code but for now we could just disable closure compiler thread and let it run in the thread than Shindig code created to run the compilation from the thread pool.
>
> Dan Dumont wrote:
> The closure threads would be blocked from growing beyond the size of the shindig thread pool though, right? We only ever compile one file with any given compiler instance.
>
> In any case, I think this is a good change. No need to spawn more threads.
Dan, that's correct. After Dan's fix before it would only grow up to the size of Shindig thread pool but it creates additional child thread per thread worker created by Shindig which is not efficient.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11577
-----------------------------------------------------------
On Sept. 15, 2012, 4:58 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2012, 4:58 p.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Dan Dumont <dd...@us.ibm.com>.
> On Sept. 15, 2012, 2:52 p.m., Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java, line 263
> > <https://reviews.apache.org/r/7122/diff/1/?file=155595#file155595line263>
> >
> > The javadoc for this method states "Disable threads. This is for clients that run on AppEngine and don't have threads." That concerns me, as this is certainly not the case we're in. Is the javadoc incomplete?
>
> Henry Saputra wrote:
> Its kinda misleading bc internally the closure JS compiler doe not use thread pool. So when compiling large number of JS resources it could spawn a lot of threads.
>
> I am planning to contribute code to add thread pool support to the code but for now we could just disable closure compiler thread and let it run in the thread than Shindig code created to run the compilation from the thread pool.
The closure threads would be blocked from growing beyond the size of the shindig thread pool though, right? We only ever compile one file with any given compiler instance.
In any case, I think this is a good change. No need to spawn more threads.
- Dan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11577
-----------------------------------------------------------
On Sept. 15, 2012, 4:58 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2012, 4:58 p.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, and Stanton Sievers.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
> On Sept. 15, 2012, 2:52 p.m., Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java, line 66
> > <https://reviews.apache.org/r/7122/diff/1/?file=155595#file155595line66>
> >
> > Is this what came from the closure code? Can you add some javadoc to make that clear.
> >
> > Maybe also make this protected or have a protected getter so people implementing their own JsCompiler can control the stack size more easily.
This is what come from closure code. I am actually will just move this to shindig.properties to make it easier to configure.
> On Sept. 15, 2012, 2:52 p.m., Stanton Sievers wrote:
> > trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java, line 263
> > <https://reviews.apache.org/r/7122/diff/1/?file=155595#file155595line263>
> >
> > The javadoc for this method states "Disable threads. This is for clients that run on AppEngine and don't have threads." That concerns me, as this is certainly not the case we're in. Is the javadoc incomplete?
Its kinda misleading bc internally the closure JS compiler doe not use thread pool. So when compiling large number of JS resources it could spawn a lot of threads.
I am planning to contribute code to add thread pool support to the code but for now we could just disable closure compiler thread and let it run in the thread than Shindig code created to run the compilation from the thread pool.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11577
-----------------------------------------------------------
On Sept. 14, 2012, 11:33 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2012, 11:33 p.m.)
>
>
> Review request for shindig, Ryan Baxter and Dan Dumont.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Stanton Sievers <si...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/#review11577
-----------------------------------------------------------
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java
<https://reviews.apache.org/r/7122/#comment24930>
Is this what came from the closure code? Can you add some javadoc to make that clear.
Maybe also make this protected or have a protected getter so people implementing their own JsCompiler can control the stack size more easily.
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java
<https://reviews.apache.org/r/7122/#comment24931>
The javadoc for this method states "Disable threads. This is for clients that run on AppEngine and don't have threads." That concerns me, as this is certainly not the case we're in. Is the javadoc incomplete?
- Stanton Sievers
On Sept. 14, 2012, 11:33 p.m., Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7122/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2012, 11:33 p.m.)
>
>
> Review request for shindig, Ryan Baxter and Dan Dumont.
>
>
> Description
> -------
>
> Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
> Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
>
> But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
>
>
> Diffs
> -----
>
> trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
>
> Diff: https://reviews.apache.org/r/7122/diff/
>
>
> Testing
> -------
>
> Run smoke test with common container with debug sets to false to run the closure js compiler.
>
>
> Thanks,
>
> Henry Saputra
>
>
Re: Review Request: Disabled thread in the closure compiler code and add
thread management in the Shindig ClosureJSCompiler class
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7122/
-----------------------------------------------------------
(Updated Sept. 14, 2012, 11:33 p.m.)
Review request for shindig, Ryan Baxter and Dan Dumont.
Description
-------
Disable thread in the closure compiler code since Shindig manage the thread in the ClosureJSCompiler class via ExecuterService framework.
Otherwise internally the closure compiler create another thread then execute the compile in that new thread instead
But we need to create the thread big enough stack to handle the compilation. Copy the max stack size from the closure compiler code.
Diffs
-----
trunk/java/gadgets/src/main/java16/org/apache/shindig/gadgets/rewrite/js/ClosureJsCompiler.java 1384587
Diff: https://reviews.apache.org/r/7122/diff/
Testing
-------
Run smoke test with common container with debug sets to false to run the closure js compiler.
Thanks,
Henry Saputra