From cf0368cc6dc75cf7b91519a34e542f0ddc74787e Mon Sep 17 00:00:00 2001 From: Nicolas Lalevee Date: Mon, 28 Jun 2010 18:23:31 +0000 Subject: [PATCH] Fix a perf issue with : it now loads only necessary resources git-svn-id: https://svn.apache.org/repos/asf/ant/core/trunk@958669 13f79535-47bb-0310-9956-ffa450edef68 --- WHATSNEW | 5 + .../AbstractResourceCollectionWrapper.java | 210 ++++++++++++++++++ .../BaseResourceCollectionWrapper.java | 163 +------------- .../LazyResourceCollectionWrapper.java | 157 +++++++++++++ .../tools/ant/types/resources/Restrict.java | 20 +- .../resources/LazyResourceCollectionTest.java | 180 +++++++++++++++ 6 files changed, 563 insertions(+), 172 deletions(-) create mode 100644 src/main/org/apache/tools/ant/types/resources/AbstractResourceCollectionWrapper.java create mode 100644 src/main/org/apache/tools/ant/types/resources/LazyResourceCollectionWrapper.java create mode 100644 src/tests/junit/org/apache/tools/ant/types/resources/LazyResourceCollectionTest.java diff --git a/WHATSNEW b/WHATSNEW index faceefe31..81be81546 100644 --- a/WHATSNEW +++ b/WHATSNEW @@ -81,6 +81,11 @@ Fixed bugs: case-insensitive file system. Bugzilla Report 49041. + * The resource collection was checking every resource even if + we actually just want the first one, like in the exemple of use of + resourcelist in the documentation (getting the first available resource + from a mirror list). + Other changes: -------------- diff --git a/src/main/org/apache/tools/ant/types/resources/AbstractResourceCollectionWrapper.java b/src/main/org/apache/tools/ant/types/resources/AbstractResourceCollectionWrapper.java new file mode 100644 index 000000000..443c323d4 --- /dev/null +++ b/src/main/org/apache/tools/ant/types/resources/AbstractResourceCollectionWrapper.java @@ -0,0 +1,210 @@ +/* + * 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.tools.ant.types.resources; + +import java.io.File; +import java.util.Iterator; +import java.util.Stack; + +import org.apache.tools.ant.BuildException; +import org.apache.tools.ant.Project; +import org.apache.tools.ant.types.DataType; +import org.apache.tools.ant.types.Resource; +import org.apache.tools.ant.types.ResourceCollection; + +/** + * Base class for a ResourceCollection that wraps a single nested + * ResourceCollection. + * @since Ant 1.8.2 + */ +public abstract class AbstractResourceCollectionWrapper + extends DataType implements ResourceCollection, Cloneable { + private static final String ONE_NESTED_MESSAGE + = " expects exactly one nested resource collection."; + + private ResourceCollection rc; + private boolean cache = true; + + /** + * Set whether to cache collections. + * @param b boolean cache flag. + */ + public synchronized void setCache(boolean b) { + cache = b; + } + + /** + * Learn whether to cache collections. Default is true. + * @return boolean cache flag. + */ + public synchronized boolean isCache() { + return cache; + } + + /** + * Add a ResourceCollection to the container. + * @param c the ResourceCollection to add. + * @throws BuildException on error. + */ + public synchronized void add(ResourceCollection c) throws BuildException { + if (isReference()) { + throw noChildrenAllowed(); + } + if (c == null) { + return; + } + if (rc != null) { + throw oneNested(); + } + rc = c; + if (Project.getProject(rc) == null) { + Project p = getProject(); + if (p != null) { + p.setProjectReference(rc); + } + } + setChecked(false); + } + + /** + * Fulfill the ResourceCollection contract. + * @return an Iterator of Resources. + */ + public final synchronized Iterator iterator() { + if (isReference()) { + return ((AbstractResourceCollectionWrapper) getCheckedRef()).iterator(); + } + dieOnCircularReference(); + return new FailFast(this, createIterator()); + } + + /** + * Do create an iterator on the resource collection. The creation + * of the iterator is allowed to not be thread safe whereas the iterator + * itself should. The returned iterator will be wrapped into the FailFast + * one. + * + * @return the iterator on the resource collection + */ + protected abstract Iterator createIterator(); + + /** + * Fulfill the ResourceCollection contract. + * @return number of elements as int. + */ + public synchronized int size() { + if (isReference()) { + return ((AbstractResourceCollectionWrapper) getCheckedRef()).size(); + } + dieOnCircularReference(); + return getSize(); + } + + /** + * Do compute the size of the resource collection. The implementation of + * this function is allowed to be not thread safe. + * + * @return + */ + protected abstract int getSize(); + + /** + * Fulfill the ResourceCollection contract. + * @return whether this is a filesystem-only resource collection. + */ + public synchronized boolean isFilesystemOnly() { + if (isReference()) { + return ((BaseResourceCollectionContainer) getCheckedRef()).isFilesystemOnly(); + } + dieOnCircularReference(); + + if (rc == null || rc.isFilesystemOnly()) { + return true; + } + /* now check each Resource in case the child only + lets through files from any children IT may have: */ + for (Iterator i = createIterator(); i.hasNext();) { + Resource r = (Resource) i.next(); + if (r.as(FileProvider.class) == null) { + return false; + } + } + return true; + } + + /** + * Overrides the version of DataType to recurse on all DataType + * child elements that may have been added. + * @param stk the stack of data types to use (recursively). + * @param p the project to use to dereference the references. + * @throws BuildException on error. + */ + protected synchronized void dieOnCircularReference(Stack stk, Project p) + throws BuildException { + if (isChecked()) { + return; + } + if (isReference()) { + super.dieOnCircularReference(stk, p); + } else { + if (rc instanceof DataType) { + pushAndInvokeCircularReferenceCheck((DataType) rc, stk, p); + } + setChecked(true); + } + } + + /** + * Get the nested ResourceCollection. + * @return a ResourceCollection. + * @throws BuildException if no nested ResourceCollection has been provided. + */ + protected final synchronized ResourceCollection getResourceCollection() { + dieOnCircularReference(); + if (rc == null) { + throw oneNested(); + } + return rc; + } + + /** + * Format this BaseResourceCollectionWrapper as a String. + * @return a descriptive String. + */ + public synchronized String toString() { + if (isReference()) { + return getCheckedRef().toString(); + } + if (getSize() == 0) { + return ""; + } + StringBuffer sb = new StringBuffer(); + for (Iterator i = createIterator(); i.hasNext();) { + if (sb.length() > 0) { + sb.append(File.pathSeparatorChar); + } + sb.append(i.next()); + } + return sb.toString(); + } + + private BuildException oneNested() { + return new BuildException(super.toString() + ONE_NESTED_MESSAGE); + } + +} diff --git a/src/main/org/apache/tools/ant/types/resources/BaseResourceCollectionWrapper.java b/src/main/org/apache/tools/ant/types/resources/BaseResourceCollectionWrapper.java index 1de3518cf..f4b5e8709 100644 --- a/src/main/org/apache/tools/ant/types/resources/BaseResourceCollectionWrapper.java +++ b/src/main/org/apache/tools/ant/types/resources/BaseResourceCollectionWrapper.java @@ -17,16 +17,8 @@ */ package org.apache.tools.ant.types.resources; -import java.io.File; -import java.util.Stack; -import java.util.Iterator; import java.util.Collection; - -import org.apache.tools.ant.Project; -import org.apache.tools.ant.BuildException; -import org.apache.tools.ant.types.DataType; -import org.apache.tools.ant.types.Resource; -import org.apache.tools.ant.types.ResourceCollection; +import java.util.Iterator; /** * Base class for a ResourceCollection that wraps a single nested @@ -34,165 +26,24 @@ import org.apache.tools.ant.types.ResourceCollection; * @since Ant 1.7 */ public abstract class BaseResourceCollectionWrapper - extends DataType implements ResourceCollection, Cloneable { - private static final String ONE_NESTED_MESSAGE - = " expects exactly one nested resource collection."; + extends AbstractResourceCollectionWrapper { - private ResourceCollection rc; private Collection coll = null; - private boolean cache = true; - - /** - * Set whether to cache collections. - * @param b boolean cache flag. - */ - public synchronized void setCache(boolean b) { - cache = b; - } - /** - * Learn whether to cache collections. Default is true. - * @return boolean cache flag. - */ - public synchronized boolean isCache() { - return cache; + protected Iterator createIterator() { + return cacheCollection().iterator(); } - /** - * Add a ResourceCollection to the container. - * @param c the ResourceCollection to add. - * @throws BuildException on error. - */ - public synchronized void add(ResourceCollection c) throws BuildException { - if (isReference()) { - throw noChildrenAllowed(); - } - if (c == null) { - return; - } - if (rc != null) { - throw oneNested(); - } - rc = c; - if (Project.getProject(rc) == null) { - Project p = getProject(); - if (p != null) { - p.setProjectReference(rc); - } - } - setChecked(false); - } - - /** - * Fulfill the ResourceCollection contract. - * @return an Iterator of Resources. - */ - public final synchronized Iterator iterator() { - if (isReference()) { - return ((BaseResourceCollectionWrapper) getCheckedRef()).iterator(); - } - dieOnCircularReference(); - return new FailFast(this, cacheCollection().iterator()); - } - - /** - * Fulfill the ResourceCollection contract. - * @return number of elements as int. - */ - public synchronized int size() { - if (isReference()) { - return ((BaseResourceCollectionWrapper) getCheckedRef()).size(); - } - dieOnCircularReference(); + protected int getSize() { return cacheCollection().size(); } - /** - * Fulfill the ResourceCollection contract. - * @return whether this is a filesystem-only resource collection. - */ - public synchronized boolean isFilesystemOnly() { - if (isReference()) { - return ((BaseResourceCollectionContainer) getCheckedRef()).isFilesystemOnly(); - } - dieOnCircularReference(); - - if (rc == null || rc.isFilesystemOnly()) { - return true; - } - /* now check each Resource in case the child only - lets through files from any children IT may have: */ - for (Iterator i = cacheCollection().iterator(); i.hasNext();) { - Resource r = (Resource) i.next(); - if (r.as(FileProvider.class) == null) { - return false; - } - } - return true; - } - - /** - * Overrides the version of DataType to recurse on all DataType - * child elements that may have been added. - * @param stk the stack of data types to use (recursively). - * @param p the project to use to dereference the references. - * @throws BuildException on error. - */ - protected synchronized void dieOnCircularReference(Stack stk, Project p) - throws BuildException { - if (isChecked()) { - return; - } - if (isReference()) { - super.dieOnCircularReference(stk, p); - } else { - if (rc instanceof DataType) { - pushAndInvokeCircularReferenceCheck((DataType) rc, stk, p); - } - setChecked(true); - } - } - - /** - * Get the nested ResourceCollection. - * @return a ResourceCollection. - * @throws BuildException if no nested ResourceCollection has been provided. - */ - protected final synchronized ResourceCollection getResourceCollection() { - dieOnCircularReference(); - if (rc == null) { - throw oneNested(); - } - return rc; - } - /** * Template method for subclasses to return a Collection of Resources. * @return Collection. */ protected abstract Collection getCollection(); - /** - * Format this BaseResourceCollectionWrapper as a String. - * @return a descriptive String. - */ - public synchronized String toString() { - if (isReference()) { - return getCheckedRef().toString(); - } - if (cacheCollection().size() == 0) { - return ""; - } - StringBuffer sb = new StringBuffer(); - for (Iterator i = coll.iterator(); i.hasNext();) { - if (sb.length() > 0) { - sb.append(File.pathSeparatorChar); - } - sb.append(i.next()); - } - return sb.toString(); - } - private synchronized Collection cacheCollection() { if (coll == null || !isCache()) { coll = getCollection(); @@ -200,8 +51,4 @@ public abstract class BaseResourceCollectionWrapper return coll; } - private BuildException oneNested() { - return new BuildException(super.toString() + ONE_NESTED_MESSAGE); - } - } diff --git a/src/main/org/apache/tools/ant/types/resources/LazyResourceCollectionWrapper.java b/src/main/org/apache/tools/ant/types/resources/LazyResourceCollectionWrapper.java new file mode 100644 index 000000000..c30386628 --- /dev/null +++ b/src/main/org/apache/tools/ant/types/resources/LazyResourceCollectionWrapper.java @@ -0,0 +1,157 @@ +package org.apache.tools.ant.types.resources; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.tools.ant.types.Resource; + +/** + * Resource collection which load underlying resource collection only on demand + * with support for caching + */ +public class LazyResourceCollectionWrapper extends + AbstractResourceCollectionWrapper { + + /** List of cached resources */ + private List cachedResources = new ArrayList(); + + private FilteringIterator filteringIterator; + + protected Iterator createIterator() { + Iterator iterator; + if (isCache()) { + if (filteringIterator == null) { + // no worry of thread safety here, see function's contract + filteringIterator = new FilteringIterator( + getResourceCollection().iterator()); + } + iterator = new CachedIterator(filteringIterator); + } else { + iterator = new FilteringIterator(getResourceCollection().iterator()); + } + return iterator; + } + + protected int getSize() { + // to compute the size, just iterate: the iterator will take care of + // caching + Iterator it = createIterator(); + int size = 0; + while (it.hasNext()) { + it.next(); + size++; + } + return size; + } + + /** + * Specify if the resource should be filtered or not. This function should + * be overrided in order to define the filtering algorithm + * + * @param r + * @return + */ + protected boolean filterResource(Resource r) { + return false; + } + + private class FilteringIterator implements Iterator { + + Resource next = null; + + boolean ended = false; + + protected final Iterator it; + + public FilteringIterator(Iterator it) { + this.it = it; + } + + public boolean hasNext() { + if (ended) { + return false; + } + while (next == null) { + if (!it.hasNext()) { + ended = true; + return false; + } + next = (Resource) it.next(); + if (filterResource(next)) { + next = null; + } + } + return true; + } + + public Object next() { + if (!hasNext()) { + throw new UnsupportedOperationException(); + } + Resource r = next; + next = null; + return r; + } + + public void remove() { + throw new UnsupportedOperationException(); + } + } + + /** + * Iterator that will put in the shared cache array list the selected + * resources + */ + private class CachedIterator implements Iterator { + + int cusrsor = 0; + + private final Iterator it; + + /** + * Default constructor + * + * @param it + * the iterator which will provide the resources to put in + * cache + */ + public CachedIterator(Iterator it) { + this.it = it; + } + + public boolean hasNext() { + synchronized (cachedResources) { + // have we already cached the next entry ? + if (cachedResources.size() > cusrsor) { + return true; + } + // does the wrapped iterator any more resource ? + if (!it.hasNext()) { + return false; + } + // put in cache the next resource + Resource r = (Resource) it.next(); + cachedResources.add(r); + } + return true; + } + + public Object next() { + // first check that we have some to deliver + if (!hasNext()) { + throw new NoSuchElementException(); + } + synchronized (cachedResources) { + // return the cached entry as hasNext should have put one for + // this iterator + return cachedResources.get(cusrsor++); + } + } + + public void remove() { + throw new UnsupportedOperationException(); + } + } +} diff --git a/src/main/org/apache/tools/ant/types/resources/Restrict.java b/src/main/org/apache/tools/ant/types/resources/Restrict.java index 011813ef5..d101687ca 100644 --- a/src/main/org/apache/tools/ant/types/resources/Restrict.java +++ b/src/main/org/apache/tools/ant/types/resources/Restrict.java @@ -17,8 +17,6 @@ */ package org.apache.tools.ant.types.resources; -import java.util.ArrayList; -import java.util.Collection; import java.util.Iterator; import java.util.Stack; @@ -37,23 +35,17 @@ import org.apache.tools.ant.types.resources.selectors.ResourceSelectorContainer; public class Restrict extends ResourceSelectorContainer implements ResourceCollection { - private BaseResourceCollectionWrapper w = new BaseResourceCollectionWrapper() { + private LazyResourceCollectionWrapper w = new LazyResourceCollectionWrapper() { /** * Restrict the nested ResourceCollection based on the nested selectors. - * @return a Collection of Resources. */ - protected Collection getCollection() { - ArrayList result = new ArrayList(); -outer: for (Iterator ri = w.getResourceCollection().iterator(); ri.hasNext();) { - Resource r = (Resource) ri.next(); - for (Iterator i = getSelectors(); i.hasNext();) { - if (!((ResourceSelector) (i.next())).isSelected(r)) { - continue outer; - } + protected boolean filterResource(Resource r) { + for (Iterator i = getSelectors(); i.hasNext();) { + if (!((ResourceSelector) (i.next())).isSelected(r)) { + return true; } - result.add(r); } - return result; + return false; } }; diff --git a/src/tests/junit/org/apache/tools/ant/types/resources/LazyResourceCollectionTest.java b/src/tests/junit/org/apache/tools/ant/types/resources/LazyResourceCollectionTest.java new file mode 100644 index 000000000..507130248 --- /dev/null +++ b/src/tests/junit/org/apache/tools/ant/types/resources/LazyResourceCollectionTest.java @@ -0,0 +1,180 @@ +/* + * 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.tools.ant.types.resources; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; + +import junit.framework.TestCase; + +import org.apache.tools.ant.types.Resource; +import org.apache.tools.ant.types.ResourceCollection; + +public class LazyResourceCollectionTest extends TestCase { + + private class StringResourceCollection implements ResourceCollection { + List resources = Arrays.asList(new Resource[] {}); + + List createdIterators = new ArrayList(); + + public int size() { + return resources.size(); + } + + public Iterator iterator() { + StringResourceIterator it = new StringResourceIterator(); + createdIterators.add(it); + return it; + } + + public boolean isFilesystemOnly() { + return false; + } + } + + private class StringResourceIterator implements Iterator { + int cursor = 0; + + public void remove() { + throw new UnsupportedOperationException(); + } + + public Object next() { + if (cursor < 3) { + cursor++; + return new StringResource("r" + cursor); + } + return null; + } + + public boolean hasNext() { + return cursor < 3; + } + } + + public void testLazyLoading() throws Exception { + StringResourceCollection collectionTest = new StringResourceCollection(); + LazyResourceCollectionWrapper lazyCollection = new LazyResourceCollectionWrapper(); + lazyCollection.add(collectionTest); + + Iterator it = lazyCollection.iterator(); + assertOneCreatedIterator(collectionTest); + StringResourceIterator stringResourceIterator = (StringResourceIterator) collectionTest.createdIterators + .get(0); + assertEquals("A resource was loaded without iterating", 1, + stringResourceIterator.cursor); + + StringResource r = (StringResource) it.next(); + assertOneCreatedIterator(collectionTest); + assertEquals("r1", r.getValue()); + assertEquals("Iterating once load more than 1 resource", 2, + stringResourceIterator.cursor); + + r = (StringResource) it.next(); + assertOneCreatedIterator(collectionTest); + assertEquals("r2", r.getValue()); + assertEquals("Iterating twice load more than 2 resources", 3, + stringResourceIterator.cursor); + + r = (StringResource) it.next(); + assertOneCreatedIterator(collectionTest); + assertEquals("r3", r.getValue()); + assertEquals("Iterating 3 times load more than 3 resources", 3, + stringResourceIterator.cursor); + + try { + it.next(); + fail("NoSuchElementException shoudl have been raised"); + } catch (NoSuchElementException e) { + // ok + } + } + + private void assertOneCreatedIterator( + StringResourceCollection testCollection) { + assertEquals("More than one iterator has been created", 1, + testCollection.createdIterators.size()); + } + + public void testCaching() throws Exception { + StringResourceCollection collectionTest = new StringResourceCollection(); + LazyResourceCollectionWrapper lazyCollection = new LazyResourceCollectionWrapper(); + lazyCollection.add(collectionTest); + + assertTrue(lazyCollection.isCache()); + Iterator it1 = lazyCollection.iterator(); + assertOneCreatedIterator(collectionTest); + Iterator it2 = lazyCollection.iterator(); + assertOneCreatedIterator(collectionTest); + + StringResourceIterator stringResourceIterator = (StringResourceIterator) collectionTest.createdIterators + .get(0); + assertEquals("A resource was loaded without iterating", 1, + stringResourceIterator.cursor); + + StringResource r = (StringResource) it1.next(); + assertEquals("r1", r.getValue()); + assertEquals("Iterating once load more than 1 resource", 2, + stringResourceIterator.cursor); + + r = (StringResource) it2.next(); + assertEquals("r1", r.getValue()); + assertEquals( + "The second iterator did not lookup in the cache for a resource", + 2, stringResourceIterator.cursor); + + r = (StringResource) it2.next(); + assertEquals("r2", r.getValue()); + assertEquals("Iterating twice load more than 2 resources", 3, + stringResourceIterator.cursor); + + r = (StringResource) it1.next(); + assertEquals("r2", r.getValue()); + assertEquals( + "The first iterator did not lookup in the cache for a resource", + 3, stringResourceIterator.cursor); + + r = (StringResource) it2.next(); + assertEquals("r3", r.getValue()); + assertEquals("Iterating 3 times load more than 3 resources", 3, + stringResourceIterator.cursor); + + r = (StringResource) it1.next(); + assertEquals("r3", r.getValue()); + assertEquals( + "The first iterator did not lookup in the cache for a resource", + 3, stringResourceIterator.cursor); + + try { + it1.next(); + fail("NoSuchElementException should have been raised"); + } catch (NoSuchElementException e) { + // ok + } + + try { + it2.next(); + fail("NoSuchElementException should have been raised"); + } catch (NoSuchElementException e) { + // ok + } + } +}