On Thu, Dec 4, 2008 at 8:26 PM, Henning P. Schmiedehausen < [EMAIL PROTECTED]> wrote:
> [EMAIL PROTECTED] writes: > > >Author: etnu > >Date: Thu Dec 4 19:01:23 2008 > >New Revision: 723563 > > >URL: http://svn.apache.org/viewvc?rev=723563&view=rev > >Log: > >Two changes to substantially reduce garbage generation: > > Why are these changes not two separate commits? Are they related? Other > than that they reduce "garbage generation"? These two bits of code represent between 50 and 90% of the allocations in a production deployment, which in turn causes substantially more gc activity yielding lots of wasted resources. They could have been separate changes, sure. > > The javadoc directly contradicts the change. Indeed. > > > >-public class DefaultMessageBundleFactory extends > AbstractMessageBundleFactory { > >+public class DefaultMessageBundleFactory implements MessageBundleFactory > { > > Is the AbstractMessageBundleFactory still used? If not, can it go? It is gone, as mentioned in the commit message. > >+ private static final Locale ALL_ALL = new Locale("all", "ALL"); > > public static final String CACHE_NAME = "messageBundles"; > > static final Logger LOG = > Logger.getLogger(DefaultMessageBundleFactory.class.getName()); > > private final HttpFetcher fetcher; > >- private final SoftExpiringCache<Uri, MessageBundle> cache; > >+ final SoftExpiringCache<String, MessageBundle> cache; > > Use a getter. The usage of direct field access in the tests will lead to > brittleness. (see DefaultMessageBundleFactoryTest) It'd be a package private getter in any case (we only care about reading it in the test), so I don't really think it matters much one way or another. > > > > private final long refresh; > > > > @Inject > >@@ -50,37 +57,37 @@ > > >Modified: > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java > >URL: > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java?rev=723563&r1=723562&r2=723563&view=diff > > >============================================================================== > >--- > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java > (original) > >+++ > incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java > Thu Dec 4 19:01:23 2008 > >@@ -234,13 +234,24 @@ > > } > > } > > > >- StringBuilder inlineJs = new StringBuilder(); > >- > > // Inline any libs that weren't forced. The ugly context switch > between inline and external > > // Js is needed to allow both inline and external scripts declared in > feature.xml. > > String container = context.getContainer(); > > Collection<GadgetFeature> features = getFeatures(spec, forced); > > > >+ // Precalculate the maximum length in order to avoid excessive > garbage generation. > >+ int size = 0; > >+ for (GadgetFeature feature : features) { > >+ for (JsLibrary library : > feature.getJsLibraries(RenderingContext.GADGET, container)) { > >+ if (library.getType().equals(JsLibrary.Type.URL)) { > >+ size += library.getContent().length(); > >+ } > >+ } > >+ } > >+ > >+ // Really inexact. > >+ StringBuilder inlineJs = new StringBuilder(size); > >+ > > How much garbage are we talking about? Is that significant (and how often > is that code run)? Every single gadget render, about 10% of allocations. > > > > > >- private static final int MAX_AGE = -10000; > >+ private static final int MAX_AGE = 10000; > > Why this change? To make the caching logic tests work. Previously the value didn't actually matter, now it does.
