Fix a perf issue with <restrict>: 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

@@ -81,6 +81,11 @@ Fixed bugs:
    case-insensitive file system.
    Bugzilla Report 49041.
 
+ * The <restrict> 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:
 --------------
 

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 <code>true</code>.
+     * @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 <code>String</code>.
+     */
+    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);
+    }
+
+}

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 <code>true</code>.
-     * @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 <code>String</code>.
-     */
-    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);
-    }
-
 }

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();
+        }
+    }
+}

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;
         }
     };
 

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
+        }
+    }
+}