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:
1. Fixed a bug in the serialization of message bundles to JSON. The
serialization here was effectively never cached in spite of Louis Ryan's
prevous patch to do so (his patch actually made the problem worse without
realizing it), which turned out to be producing around 30% of garbage in the
entire system. As has been noted before, the org.json library really doesn't
scale well and we should probably replace it entirely. The biggest problem is
with serialization efficiency.
2. Pre-allocated the buffer used for injecting javascript features into the
rendered output. This would likely have no impact on containers that always
force the full set of libraries, but on other containers the savings could be
substantial.
Note that this removes the no longer necessary AbstractMessageBundle. Now that
custom cache implementations can be provided, it's not a needed class. Custom
(non HttpFetcher) based lookup can be achieved by overriding fetch* methods.
Removed:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AbstractMessageBundleFactory.java
Modified:
incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/cache/SoftExpiringCache.java
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingContentRewriter.java
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultMessageBundleFactoryTest.java
Modified:
incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/cache/SoftExpiringCache.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/cache/SoftExpiringCache.java?rev=723563&r1=723562&r2=723563&view=diff
==============================================================================
---
incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/cache/SoftExpiringCache.java
(original)
+++
incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/cache/SoftExpiringCache.java
Thu Dec 4 19:01:23 2008
@@ -93,7 +93,7 @@
* Set a new time source. Used for testing, so package-private.
* @param timeSource New time source to use.
*/
- void setTimeSource(TimeSource timeSource) {
+ public void setTimeSource(TimeSource timeSource) {
this.timeSource = timeSource;
}
Modified:
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java?rev=723563&r1=723562&r2=723563&view=diff
==============================================================================
---
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
(original)
+++
incubator/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultMessageBundleFactory.java
Thu Dec 4 19:01:23 2008
@@ -21,10 +21,12 @@
import org.apache.shindig.common.cache.Cache;
import org.apache.shindig.common.cache.CacheProvider;
import org.apache.shindig.common.cache.SoftExpiringCache;
+import org.apache.shindig.common.cache.SoftExpiringCache.CachedObject;
import org.apache.shindig.common.uri.Uri;
import org.apache.shindig.gadgets.http.HttpFetcher;
import org.apache.shindig.gadgets.http.HttpRequest;
import org.apache.shindig.gadgets.http.HttpResponse;
+import org.apache.shindig.gadgets.spec.GadgetSpec;
import org.apache.shindig.gadgets.spec.LocaleSpec;
import org.apache.shindig.gadgets.spec.MessageBundle;
@@ -32,17 +34,22 @@
import com.google.inject.Singleton;
import com.google.inject.name.Named;
+import java.util.Locale;
import java.util.logging.Logger;
/**
* Default implementation of a message bundle factory.
+ *
+ * Containers wishing to implement custom bundle fetching behavior should
override
+ * [EMAIL PROTECTED] #fetchBundle}.
*/
@Singleton
-public class DefaultMessageBundleFactory extends AbstractMessageBundleFactory {
+public class DefaultMessageBundleFactory implements MessageBundleFactory {
+ 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;
private final long refresh;
@Inject
@@ -50,37 +57,37 @@
CacheProvider cacheProvider,
@Named("shindig.cache.xml.refreshInterval") long refresh) {
this.fetcher = fetcher;
- Cache<Uri, MessageBundle> baseCache =
cacheProvider.createCache(CACHE_NAME);
- this.cache = new SoftExpiringCache<Uri, MessageBundle>(baseCache);
+ Cache<String, MessageBundle> baseCache =
cacheProvider.createCache(CACHE_NAME);
+ this.cache = new SoftExpiringCache<String, MessageBundle>(baseCache);
this.refresh = refresh;
}
- @Override
- protected MessageBundle fetchBundle(LocaleSpec locale, boolean ignoreCache)
+ public MessageBundle getBundle(GadgetSpec spec, Locale locale, boolean
ignoreCache)
throws GadgetException {
+
if (ignoreCache) {
- return fetchAndCacheBundle(locale, ignoreCache);
+ return getNestedBundle(spec, locale, true);
}
- Uri uri = locale.getMessages();
-
- SoftExpiringCache.CachedObject<MessageBundle> cached =
cache.getElement(uri);
+ String key = spec.getUrl().toString() + '.' + locale.toString();
+ CachedObject<MessageBundle> cached = cache.getElement(key);
- MessageBundle bundle = null;
+ MessageBundle bundle;
if (cached == null || cached.isExpired) {
try {
- bundle = fetchAndCacheBundle(locale, ignoreCache);
+ bundle = getNestedBundle(spec, locale, ignoreCache);
} catch (GadgetException e) {
// Enforce negative caching.
if (cached != null) {
+ LOG.info("MessageBundle fetch failed for " + key + " - using
cached.");
bundle = cached.obj;
} else {
// We create this dummy spec to avoid the cost of re-parsing when a
remote site is out.
+ LOG.info("MessageBundle fetch failed for " + key + " - using
default.");
bundle = MessageBundle.EMPTY;
}
- LOG.info("MessageBundle fetch failed for " + uri + " - using cached.");
- cache.addElement(uri, bundle, refresh);
}
+ cache.addElement(key, bundle, refresh);
} else {
bundle = cached.obj;
}
@@ -88,7 +95,38 @@
return bundle;
}
- private MessageBundle fetchAndCacheBundle(LocaleSpec locale, boolean
ignoreCache)
+ private MessageBundle getNestedBundle(GadgetSpec spec, Locale locale,
boolean ignoreCache)
+ throws GadgetException {
+ MessageBundle parent = getParentBundle(spec, locale, ignoreCache);
+ MessageBundle child = null;
+ LocaleSpec localeSpec = spec.getModulePrefs().getLocale(locale);
+ if (localeSpec == null) {
+ return parent == null ? MessageBundle.EMPTY : parent;
+ }
+ Uri messages = localeSpec.getMessages();
+ if (messages == null || messages.toString().length() == 0) {
+ child = localeSpec.getMessageBundle();
+ } else {
+ child = fetchBundle(localeSpec, ignoreCache);
+ }
+ return new MessageBundle(parent, child);
+ }
+
+ private MessageBundle getParentBundle(GadgetSpec spec, Locale locale,
boolean ignoreCache)
+ throws GadgetException {
+ if (locale.getLanguage().equalsIgnoreCase("all")) {
+ // Top most locale already.
+ return null;
+ }
+
+ if (locale.getCountry().equalsIgnoreCase("ALL")) {
+ return getBundle(spec, ALL_ALL, ignoreCache);
+ }
+
+ return getBundle(spec, new Locale(locale.getLanguage(), "ALL"),
ignoreCache);
+ }
+
+ protected MessageBundle fetchBundle(LocaleSpec locale, boolean ignoreCache)
throws GadgetException {
Uri url = locale.getMessages();
HttpRequest request = new HttpRequest(url).setIgnoreCache(ignoreCache);
@@ -104,7 +142,6 @@
}
MessageBundle bundle = new MessageBundle(locale,
response.getResponseAsString());
- cache.addElement(url, bundle, refresh);
return bundle;
}
}
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);
+
for (GadgetFeature feature : features) {
for (JsLibrary library : feature.getJsLibraries(RenderingContext.GADGET,
container)) {
if (library.getType().equals(JsLibrary.Type.URL)) {
Modified:
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultMessageBundleFactoryTest.java
URL:
http://svn.apache.org/viewvc/incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultMessageBundleFactoryTest.java?rev=723563&r1=723562&r2=723563&view=diff
==============================================================================
---
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultMessageBundleFactoryTest.java
(original)
+++
incubator/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultMessageBundleFactoryTest.java
Thu Dec 4 19:01:23 2008
@@ -23,10 +23,13 @@
import static org.easymock.EasyMock.verify;
import static org.easymock.classextension.EasyMock.replay;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import org.apache.shindig.common.cache.Cache;
import org.apache.shindig.common.cache.CacheProvider;
import org.apache.shindig.common.cache.LruCacheProvider;
import org.apache.shindig.common.uri.Uri;
+import org.apache.shindig.common.util.TimeSource;
import org.apache.shindig.gadgets.http.HttpFetcher;
import org.apache.shindig.gadgets.http.HttpRequest;
import org.apache.shindig.gadgets.http.HttpResponse;
@@ -38,6 +41,7 @@
import org.junit.Test;
import java.util.Locale;
+import java.util.concurrent.atomic.AtomicLong;
/**
* Tests for DefaultMessageBundleFactory
@@ -80,11 +84,13 @@
"<Content type='html'/>" +
"</Module>";
- private static final int MAX_AGE = -10000;
+ private static final int MAX_AGE = 10000;
private final HttpFetcher fetcher =
EasyMock.createNiceMock(HttpFetcher.class);
private final CacheProvider cacheProvider = new LruCacheProvider(10);
- private final MessageBundleFactory bundleFactory
+ private final Cache<String, MessageBundle> cache
+ = cacheProvider.createCache(DefaultMessageBundleFactory.CACHE_NAME);
+ private final DefaultMessageBundleFactory bundleFactory
= new DefaultMessageBundleFactory(fetcher, cacheProvider, MAX_AGE);
private final GadgetSpec gadgetSpec;
@@ -111,6 +117,18 @@
}
@Test
+ public void getBundleFromCache() throws Exception {
+ HttpResponse response = new HttpResponse(BASIC_BUNDLE);
+ expect(fetcher.fetch(isA(HttpRequest.class))).andReturn(response).once();
+ replay(fetcher);
+
+ MessageBundle bundle0 = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
+ MessageBundle bundle1 = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
+
+ assertSame("Different objects returned out of the cache", bundle0,
bundle1);
+ }
+
+ @Test
public void getParentBundle() throws Exception {
MessageBundle bundle = bundleFactory.getBundle(gadgetSpec, PARENT_LOCALE,
true);
@@ -126,6 +144,12 @@
}
@Test
+ public void ignoreCacheDoesNotStore() throws Exception {
+ bundleFactory.getBundle(gadgetSpec, new Locale("all", "ALL"), true);
+ assertEquals(0, cache.getSize());
+ }
+
+ @Test
public void badResponseServedFromCache() throws Exception {
HttpResponse expiredResponse = new HttpResponseBuilder()
.setResponse(BASIC_BUNDLE.getBytes("UTF-8"))
@@ -139,24 +163,53 @@
.andReturn(badResponse).once();
replay(fetcher);
- MessageBundle bundle = bundleFactory.getBundle(gadgetSpec, LOCALE, true);
- bundle = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
+ final AtomicLong time = new AtomicLong();
+
+ bundleFactory.cache.setTimeSource(new TimeSource() {
+ @Override
+ public long currentTimeMillis() {
+ return time.get();
+ }
+ });
+
+ time.set(System.currentTimeMillis());
+
+ MessageBundle bundle0 = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
+
+ time.set(time.get() + MAX_AGE + 1);
+
+ MessageBundle bundle1 = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
verify(fetcher);
- assertEquals(MSG_0_VALUE, bundle.getMessages().get(MSG_0_NAME));
+ assertSame("Did not respond from cache when refresh failed.", bundle0,
bundle1);
+ }
+
+ @Test
+ public void badResponseIsEmptyWhenNotInCache() throws Exception {
+ HttpResponse badResponse = HttpResponse.error();
+
+ expect(fetcher.fetch(isA(HttpRequest.class)))
+ .andReturn(badResponse).once();
+ replay(fetcher);
+
+ MessageBundle bundle = bundleFactory.getBundle(gadgetSpec, LOCALE, false);
+
+ verify(fetcher);
+
+ assertEquals(0, bundle.getMessages().size());
}
@Test
public void ttlPropagatesToFetcher() throws Exception {
CapturingFetcher capturingFetcher = new CapturingFetcher();
- MessageBundleFactory forcedTtlFactory
- = new DefaultMessageBundleFactory(capturingFetcher, cacheProvider,
10000);
+ MessageBundleFactory factory
+ = new DefaultMessageBundleFactory(capturingFetcher, cacheProvider,
MAX_AGE);
- forcedTtlFactory.getBundle(gadgetSpec, LOCALE, false);
+ factory.getBundle(gadgetSpec, LOCALE, false);
- assertEquals(10, capturingFetcher.request.getCacheTtl());
+ assertEquals(MAX_AGE / 1000, capturingFetcher.request.getCacheTtl());
}
private static class CapturingFetcher implements HttpFetcher {
