YUI compression doesn't cache and leaves streams open (patch included)

[GoogleCodeExporter]

While working with WiQuery I noticed the YUI compression of JavaScript and CSS 
files in org/odlabs/wiquery/core/commons/compressed have 2 problems:

- Streams are left open after a client receives a compressed file. This results 
in high memory usage and stacktraces on undeploy/redeploy.
- The SoftReference caching the compressed data wasn't used (properly). In 
fact, every file was compressed _twice_ for each request!

I've fixed both problems, increasing performance for compressed files by 
500-800% in both requests per second and response time.

A patch for the trunk is attached.

Original issue reported on code.google.com by norc...@gmail.com on 22 Dec 2010 at 9:45

Attachments:

• wiquery-yui-compr-patch.txt

[GoogleCodeExporter]

Oh dear,

I will look into this immediately.

Original comment by hielke.hoeve on 22 Dec 2010 at 10:00

• Added labels: Priority-Critical, Performance
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Hi,

Really, thank you for your patch.

If Hielke is ok, we will integrate it into the trunk and we will deploy a new 
release of wiquery (1.1.3)

Original comment by roche....@gmail.com on 22 Dec 2010 at 10:33

• Added labels: Milestone-Release1.1.3

[GoogleCodeExporter]

I committed the change, I don't see how the extra wrapping resource helps 
caching as the compressedpackageresource already does this for us.

Thanks for pointing this out, it did not show up on our application statistics.

Original comment by hielke.hoeve on 22 Dec 2010 at 2:34

• Changed state: Fixed

[GoogleCodeExporter]

The changes you committed will not fix the caching issue, let me explain:

In CompressedPackageResource, getOriginalResourceStream() is called in every 
other method returning (meta)data. The current implementation of 
WiQueryYUICompressedJavaScriptResource overrides this method and returns a new 
WiQueryYUICompressedJavascriptResourceStream every time. Since the cache is an 
instance variable, a new cache will be created every time, rendering it useless.

By overriding newResourceStream() instead and adding the extra wrapping with a 
transient variable holding on to the created resourcestreams, the same instance 
will be used and the cache will work. The extra close() implementation makes 
sure the internal inputstreams are discarded and each request gets a new stream 
build from the cached data.

Original comment by norc...@gmail.com on 22 Dec 2010 at 2:48

[GoogleCodeExporter]

You can see how things behave by yourself by adding a breakpoint inside 
getCompressedContent(), for example when a new compressor is created. This will 
happen twice per resource request..

(Can't I edit my comment?)

Original comment by norc...@gmail.com on 22 Dec 2010 at 2:49

[GoogleCodeExporter]

Ah must have missed that, I see your point. Committed all of your patch, can 
you check this?

(Nope no one can, good site eh? :) )

Original comment by hielke.hoeve on 22 Dec 2010 at 3:05

[GoogleCodeExporter]

Almost, you left WiQueryYUICompressedStyleSheetResourceStream and 
WiQueryYUICompressedJavascriptResourceStream implementing IResourceStream 
instead of extending my new class WiQueryYUICompressedResourceStream, thereby 
leaving all the methods I moved to the more generic class in place.

Original comment by norc...@gmail.com on 22 Dec 2010 at 3:10

[GoogleCodeExporter]

Strange, I applied the patch and committed... Reapplied patch from the start.

Original comment by hielke.hoeve on 22 Dec 2010 at 3:34

[GoogleCodeExporter]

Thanks!

"One more thing": I introduced a potential concurrency problem with my patch. 
Haven't seen it while testing under high load, but it may be triggered in some 
situations.

The attached patch fixed this while keeping the high performance. Sorry for 
being a pain :)

Original comment by norc...@gmail.com on 22 Dec 2010 at 4:13

Attachments:

• wiquery-yui-compr-patch2.txt

[GoogleCodeExporter]

Oops.. new file.

Original comment by norc...@gmail.com on 22 Dec 2010 at 4:16

Attachments:

• wiquery-yui-compr-patch2.txt

[GoogleCodeExporter]

As most resources and streams in Wicket don't use ThreadLocal I think it is 
safe enough to see what happens with the current code. If we are having 
problems with this so will Wicket.

Original comment by hielke.hoeve on 23 Dec 2010 at 8:02

[GoogleCodeExporter]

That's because the Wicket classes don't store the original resourcestream and 
created ByteArrayInputStream at all! I've been able to trigger a concurrency 
problem by setting a breakpoint inside getInputStream() and performing 2 
concurrent requests to the same resource. 

I think it's better to act _before_ someone reports this instead of waiting for 
it to happen, so I strongly recommend adding the latest code. You can also 
remove the optimizations in getInputStream() and 
innerGetOriginalResourceStream() together with the instance variables, reducing 
performance by around 250% (caused by the repeated call of 
getOriginalResourceStream which is pretty expensive). Still better than is was 
originally though.

Original comment by norc...@gmail.com on 23 Dec 2010 at 8:43

[GoogleCodeExporter]

CompressedPackageResource does keep a reference to the resourcestream, I have 
also triggered the problem so maybe it is indeed better to apply it :)

Original comment by hielke.hoeve on 23 Dec 2010 at 9:48