WICKET-5527 Inefficient DefaultPageStore.SerializedPagesCache Add tests and fix bugs in the new PerSessionPageStore impl
Project: http://git-wip-us.apache.org/repos/asf/wicket/repo Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/1c31627e Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/1c31627e Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/1c31627e Branch: refs/heads/master Commit: 1c31627e4bfc167bc6e2e29bd642991d6b5a1dcb Parents: 25016d1 Author: Martin Tzvetanov Grigorov <mgrigo...@apache.org> Authored: Tue Mar 11 12:17:55 2014 +0200 Committer: Martin Tzvetanov Grigorov <mgrigo...@apache.org> Committed: Tue Mar 11 12:17:55 2014 +0200 ---------------------------------------------------------------------- .../wicket/pageStore/DefaultPageStore.java | 46 ++++--- .../wicket/pageStore/PerSessionPageStore.java | 52 +++++--- .../wicket/pageStore/AbstractPageStoreTest.java | 131 +++++++++++++++++++ .../wicket/pageStore/DefaultPageStoreTest.java | 31 +++++ .../apache/wicket/pageStore/NoopDataStore.java | 61 +++++++++ .../pageStore/PerSessionPageStoreTest.java | 53 ++++++++ 6 files changed, 336 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/main/java/org/apache/wicket/pageStore/DefaultPageStore.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/pageStore/DefaultPageStore.java b/wicket-core/src/main/java/org/apache/wicket/pageStore/DefaultPageStore.java index 393fac3..e574f37 100644 --- a/wicket-core/src/main/java/org/apache/wicket/pageStore/DefaultPageStore.java +++ b/wicket-core/src/main/java/org/apache/wicket/pageStore/DefaultPageStore.java @@ -303,18 +303,19 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. */ static class SerializedPagesCache implements SecondLevelPageCache<String, Integer, SerializedPage> { - private final int size; + private final int maxSize; private final ConcurrentLinkedDeque<SoftReference<SerializedPage>> cache; /** - * Construct. + * Constructor. * - * @param size + * @param maxSize + * The maximum number of entries to cache */ - public SerializedPagesCache(final int size) + public SerializedPagesCache(final int maxSize) { - this.size = size; + this.maxSize = maxSize; cache = new ConcurrentLinkedDeque<>(); } @@ -327,17 +328,18 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. @Override public SerializedPage removePage(final String sessionId, final Integer pageId) { - if (size > 0) + if (maxSize > 0) { Args.notNull(sessionId, "sessionId"); Args.notNull(pageId, "pageId"); + SerializedPage sample = new SerializedPage(sessionId, pageId, null); + for (Iterator<SoftReference<SerializedPage>> i = cache.iterator(); i.hasNext();) { SoftReference<SerializedPage> ref = i.next(); SerializedPage entry = ref.get(); - if (entry != null && entry.getPageId() == pageId && - entry.getSessionId().equals(sessionId)) + if (sample.equals(entry)) { i.remove(); return entry; @@ -356,7 +358,7 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. @Override public void removePages(String sessionId) { - if (size > 0) + if (maxSize > 0) { Args.notNull(sessionId, "sessionId"); @@ -385,17 +387,18 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. public SerializedPage getPage(String sessionId, Integer pageId) { SerializedPage result = null; - if (size > 0) + if (maxSize > 0) { Args.notNull(sessionId, "sessionId"); Args.notNull(pageId, "pageId"); + SerializedPage sample = new SerializedPage(sessionId, pageId, null); + for (Iterator<SoftReference<SerializedPage>> i = cache.iterator(); i.hasNext();) { SoftReference<SerializedPage> ref = i.next(); SerializedPage entry = ref.get(); - if (entry != null && entry.getPageId() == pageId && - entry.getSessionId().equals(sessionId)) + if (sample.equals(entry)) { i.remove(); result = entry; @@ -406,7 +409,7 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. if (result != null) { // move to top - storePage(sessionId, pageId, result); + internalStore(result); } } return result; @@ -421,7 +424,7 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. @Override public void storePage(String sessionId, Integer pageId, SerializedPage page) { - if (size > 0) + if (maxSize > 0) { Args.notNull(sessionId, "sessionId"); Args.notNull(pageId, "pageId"); @@ -438,11 +441,16 @@ public class DefaultPageStore extends AbstractCachingPageStore<DefaultPageStore. } } - cache.add(new SoftReference<>(page)); - while (cache.size() > size) - { - cache.remove(0); - } + internalStore(page); + } + } + + private void internalStore(SerializedPage page) + { + cache.push(new SoftReference<>(page)); + while (cache.size() > maxSize) + { + cache.pollLast(); } } http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/main/java/org/apache/wicket/pageStore/PerSessionPageStore.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/main/java/org/apache/wicket/pageStore/PerSessionPageStore.java b/wicket-core/src/main/java/org/apache/wicket/pageStore/PerSessionPageStore.java index 8d0c631..eaff550 100644 --- a/wicket-core/src/main/java/org/apache/wicket/pageStore/PerSessionPageStore.java +++ b/wicket-core/src/main/java/org/apache/wicket/pageStore/PerSessionPageStore.java @@ -18,6 +18,7 @@ package org.apache.wicket.pageStore; import java.lang.ref.SoftReference; import java.util.Comparator; +import java.util.Iterator; import java.util.Map; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ConcurrentSkipListMap; @@ -25,7 +26,6 @@ import java.util.concurrent.ConcurrentSkipListMap; import org.apache.wicket.page.IManageablePage; import org.apache.wicket.serialize.ISerializer; import org.apache.wicket.util.lang.Args; -import org.apache.wicket.util.time.Time; /** * A page store that uses a SecondLevelPageCache with the last N used page instances @@ -75,6 +75,8 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag } /** + * An implementation of SecondLevelPageCache that stores the last used N live page instances + * per http session. */ protected static class PagesCache implements SecondLevelPageCache<String, Integer, IManageablePage> { @@ -92,7 +94,7 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag /** * The last time this page has been used/accessed. */ - private Time accessTime; + private long accessTime; private PageValue(IManageablePage page) { @@ -102,7 +104,7 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag private PageValue(int pageId) { this.pageId = pageId; - this.accessTime = Time.now(); + this.accessTime = System.nanoTime(); } @Override @@ -114,7 +116,6 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag PageValue pageValue = (PageValue) o; return pageId == pageValue.pageId; - } @Override @@ -129,7 +130,7 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag @Override public int compare(PageValue p1, PageValue p2) { - return p1.accessTime.compareTo(p2.accessTime); + return Long.valueOf(p1.accessTime).compareTo(p2.accessTime); } } @@ -173,11 +174,17 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag ConcurrentMap<PageValue, IManageablePage> pages = pagesPerSession.get(); if (pages != null) { - PageValue pv = new PageValue(pageId); - IManageablePage page = pages.remove(pv); - if (page != null) + PageValue sample = new PageValue(pageId); + Iterator<Map.Entry<PageValue, IManageablePage>> iterator = pages.entrySet().iterator(); + while (iterator.hasNext()) { - result = page; + Map.Entry<PageValue, IManageablePage> entry = iterator.next(); + if (sample.equals(entry.getKey())) + { + result = entry.getValue(); + iterator.remove(); + break; + } } } } @@ -231,14 +238,19 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag ConcurrentSkipListMap<PageValue, IManageablePage> pages = pagesPerSession.get(); if (pages != null) { - PageValue pv = new PageValue(pageId); - Map.Entry<PageValue, IManageablePage> entry = pages.ceilingEntry(pv); - - if (entry != null) + PageValue sample = new PageValue(pageId); + Iterator<Map.Entry<PageValue,IManageablePage>> iterator = pages.entrySet().iterator(); + while (iterator.hasNext()) { - // touch the entry - entry.getKey().accessTime = Time.now(); - result = entry.getValue(); + Map.Entry<PageValue, IManageablePage> entry = iterator.next(); + + if (sample.equals(entry.getKey())) + { + // touch the entry + entry.getKey().accessTime = System.nanoTime(); + result = entry.getValue(); + break; + } } } } @@ -286,13 +298,15 @@ public class PerSessionPageStore extends AbstractCachingPageStore<IManageablePag if (pages != null) { + removePage(sessionId, pageId); + + PageValue pv = new PageValue(page); + pages.put(pv, page); + while (pages.size() > maxEntriesPerSession) { pages.pollFirstEntry(); } - - PageValue pv = new PageValue(page); - pages.put(pv, page); } } } http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/test/java/org/apache/wicket/pageStore/AbstractPageStoreTest.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/test/java/org/apache/wicket/pageStore/AbstractPageStoreTest.java b/wicket-core/src/test/java/org/apache/wicket/pageStore/AbstractPageStoreTest.java new file mode 100644 index 0000000..42cce39 --- /dev/null +++ b/wicket-core/src/test/java/org/apache/wicket/pageStore/AbstractPageStoreTest.java @@ -0,0 +1,131 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.pageStore; + +import org.apache.wicket.MockPage; +import org.apache.wicket.serialize.ISerializer; +import org.apache.wicket.serialize.java.JavaSerializer; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public abstract class AbstractPageStoreTest extends Assert +{ + protected final String sessionId = "1234567890"; + protected final int pageId = 123; + protected final ISerializer serializer = new JavaSerializer(DefaultPageStore.class.getName()); + protected final IDataStore dataStore = new NoopDataStore(); + protected int maxEntries = 1; + protected IPageStore pageStore = null; + + @Before + public void before() + { + pageStore = createPageStore(serializer, dataStore, maxEntries); + } + + protected abstract IPageStore createPageStore(ISerializer serializer, IDataStore dataStore, int maxEntries); + + @After + public void after() + { + if (pageStore != null) + { + pageStore.destroy(); + pageStore = null; + } + } + + /** + * Assert that a stored page is available to be read + */ + @Test + public void storePage() + { + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + } + + /** + * Assert that storing a page twice won't keep two entries + */ + @Test + public void storePage2() + { + int maxEntries = 10; + + pageStore = createPageStore(serializer, dataStore, maxEntries); + + pageStore.storePage(sessionId, new MockPage(pageId)); + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + + pageStore.removePage(sessionId, pageId); + + assertNull(pageStore.getPage(sessionId, pageId)); + } + + @Test + public void removePage() + { + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + + pageStore.removePage(sessionId, pageId); + + assertNull(pageStore.getPage(sessionId, pageId)); + } + + /** + * Verify that at most {@code maxEntries} per session can be put in the cache + */ + @Test + public void maxSizeSameSession() + { + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + + int pageId2 = 234; + pageStore.storePage(sessionId, new MockPage(pageId2)); + assertNull(pageStore.getPage(sessionId, pageId)); + assertNotNull(pageStore.getPage(sessionId, pageId2)); + } + + /** + * Verify that it is OK to store more pages than {@code maxEntries} + * if they are in different sessions + */ + @Test + public void maxSizeDifferentSessions() + { + String sessionId2 = "0987654321"; + + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + + pageStore.storePage(sessionId2, new MockPage(pageId)); + + assertNull(pageStore.getPage(sessionId, pageId)); + assertNotNull(pageStore.getPage(sessionId2, pageId)); + } +} http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/test/java/org/apache/wicket/pageStore/DefaultPageStoreTest.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/test/java/org/apache/wicket/pageStore/DefaultPageStoreTest.java b/wicket-core/src/test/java/org/apache/wicket/pageStore/DefaultPageStoreTest.java new file mode 100644 index 0000000..2ffa376 --- /dev/null +++ b/wicket-core/src/test/java/org/apache/wicket/pageStore/DefaultPageStoreTest.java @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.pageStore; + +import org.apache.wicket.serialize.ISerializer; + +/** + * Tests for DefaultPageStore + */ +public class DefaultPageStoreTest extends AbstractPageStoreTest +{ + @Override + protected IPageStore createPageStore(ISerializer serializer, IDataStore dataStore, int maxEntries) + { + return new DefaultPageStore(serializer, dataStore, maxEntries); + } +} http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/test/java/org/apache/wicket/pageStore/NoopDataStore.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/test/java/org/apache/wicket/pageStore/NoopDataStore.java b/wicket-core/src/test/java/org/apache/wicket/pageStore/NoopDataStore.java new file mode 100644 index 0000000..0cadaee --- /dev/null +++ b/wicket-core/src/test/java/org/apache/wicket/pageStore/NoopDataStore.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.pageStore; + +/** + * An implementation of IDataStore that does nothing + */ +public class NoopDataStore implements IDataStore +{ + @Override + public byte[] getData(String sessionId, int id) + { + return null; + } + + @Override + public void removeData(String sessionId, int id) + { + } + + @Override + public void removeData(String sessionId) + { + } + + @Override + public void storeData(String sessionId, int id, byte[] data) + { + } + + @Override + public void destroy() + { + } + + @Override + public boolean isReplicated() + { + return false; + } + + @Override + public boolean canBeAsynchronous() + { + return false; + } +} http://git-wip-us.apache.org/repos/asf/wicket/blob/1c31627e/wicket-core/src/test/java/org/apache/wicket/pageStore/PerSessionPageStoreTest.java ---------------------------------------------------------------------- diff --git a/wicket-core/src/test/java/org/apache/wicket/pageStore/PerSessionPageStoreTest.java b/wicket-core/src/test/java/org/apache/wicket/pageStore/PerSessionPageStoreTest.java new file mode 100644 index 0000000..0cd4a2a --- /dev/null +++ b/wicket-core/src/test/java/org/apache/wicket/pageStore/PerSessionPageStoreTest.java @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.wicket.pageStore; + +import org.apache.wicket.MockPage; +import org.apache.wicket.serialize.ISerializer; +import org.junit.Test; + +/** + * Tests for PerSessionPageStore + */ +public class PerSessionPageStoreTest extends AbstractPageStoreTest +{ + @Override + protected IPageStore createPageStore(ISerializer serializer, IDataStore dataStore, int maxEntries) + { + return new PerSessionPageStore(serializer, dataStore, maxEntries); + } + + /** + * Verify that it is OK to store more pages than {@code maxEntries} + * if they are in different sessions + */ + @Test + @Override + public void maxSizeDifferentSessions() + { + String sessionId2 = "0987654321"; + + pageStore.storePage(sessionId, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + + pageStore.storePage(sessionId2, new MockPage(pageId)); + + assertNotNull(pageStore.getPage(sessionId, pageId)); + assertNotNull(pageStore.getPage(sessionId2, pageId)); + } +}
