Permalink
Browse files

Improve DescendantIterator performance

  • Loading branch information...
1 parent 78e0d6e commit 73a7138c879457ae17aa035da763ac18bf63763a @rolfl rolfl committed Mar 22, 2012
@@ -69,11 +69,27 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
* @author Rolf Lear
*/
final class DescendantIterator implements IteratorIterable<Content> {
-
+
+ /** Needed to be Iterable! */
private final Parent parent;
- private LinkedList<Iterator<Content>> stack = new LinkedList<Iterator<Content>>();
+
+ /*
+ * Note, we use an Array of Object here, even through
+ * List<Iterator<Content>> would look neater, etc.
+ * Fact is, for 'hamlet', using a list for the stack takes about
+ * twice as long as using the Object[] array.
+ */
+ private Object[] stack = new Object[16];
+ private int ssize = 0;
+
+ /** The iterator that supplied to most recent next() */
private Iterator<Content> current = null;
- private Iterator<Content> following = null;
+ /** The iterator going down the tree, null unless next() returned Parent */
+ private Iterator<Content> descending = null;
+ /** The iterator going up the tree, null unless next() returned dead-end */
+ private Iterator<Content> ascending = null;
+ /** what it says... */
+ private boolean hasnext = true;
/**
* Iterator for the descendants of the supplied object.
@@ -84,10 +100,12 @@ OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
this.parent = parent;
// can trust that parent is not null, DescendantIterator is package-private.
current = parent.getContent().iterator();
+ hasnext = current.hasNext();
}
@Override
public DescendantIterator iterator() {
+ // Implement the Iterable stuff.
return new DescendantIterator(parent);
}
@@ -98,21 +116,7 @@ public DescendantIterator iterator() {
*/
@Override
public boolean hasNext() {
- if (following != null && following.hasNext()) {
- // the last content returned has un-processed child content
- return true;
- }
- if (current.hasNext()) {
- // the last content has un-iterated siblings.
- return true;
- }
- for (Iterator<Content> it : stack) {
- if (it.hasNext()) {
- // an ancestor has un-iterated siblings.
- return true;
- }
- }
- return false;
+ return hasnext;
}
/**
@@ -122,36 +126,51 @@ public boolean hasNext() {
*/
@Override
public Content next() {
- Content ret = null;
- if (following != null && following.hasNext()) {
- // The last returned content is a parent with unprocessed content
- ret = following.next();
- stack.addFirst(current);
- current = following;
- } else if (current.hasNext()) {
- // the last content returned has un-iterated siblings.
- ret = current.next();
- } else {
- // check up the ancestry for the next unprocessed sibling...
- while (ret == null && !stack.isEmpty()) {
- // while we go we pop the stack.
- current = stack.removeFirst();
- if (current.hasNext()) {
- ret = current.next();
- }
+ // set the 'current' if it needs changing.
+ if (descending != null) {
+ current = descending;
+ descending = null;
+ } else if (ascending != null) {
+ current = ascending;
+ ascending = null;
+ }
+
+ final Content ret = current.next();
+
+ // got an item to return.
+ // sort out the next state....
+ if ((ret instanceof Element) && ((Element)ret).getContentSize() > 0) {
+ // there is another descendant, and it has values.
+ // our next will be down....
+ descending = ((Element)ret).getContent().iterator();
+ if (ssize >= stack.length) {
+ stack = Arrays.copyOf(stack, ssize + 16);
}
+ stack[ssize++] = current;
+ return ret;
}
- if (ret == null) {
- throw new NoSuchElementException("Iterated off the end of the " +
- "DescendantIterator");
+ if (current.hasNext()) {
+ // our next will be along....
+ return ret;
}
- if (ret instanceof Parent) {
- following = ((Parent)ret).getContent().iterator();
- } else {
- following = null;
+ // our next will be up.
+ while (ssize > 0) {
+
+ // if the stack was generic, this would not be needed, but,
+ // the java.uti.* stack codes are too slow.
+ @SuppressWarnings("unchecked")
+ final Iterator<Content> subit = (Iterator<Content>)stack[--ssize];
+ ascending = subit;
+ stack[ssize] = null;
+ if (ascending.hasNext()) {
+ return ret;
+ }
}
+
+ ascending = null;
+ hasnext = false;
return ret;
}
@@ -164,7 +183,27 @@ public Content next() {
@Override
public void remove() {
current.remove();
- following = null;
+ // if our next move was to go down, we can't.
+ // we can go along, or up.
+ descending = null;
+ if (current.hasNext() || ascending != null) {
+ // we have a next element, or our next move was up anyway.
+ return;
+ }
+ // our next move was going to be down, or accross, but those are not
+ // possible any more, need to check up.
+ // our next will be up.
+ while (ssize > 0) {
+ @SuppressWarnings("unchecked")
+ final Iterator<Content> subit = (Iterator<Content>)stack[--ssize];
+ stack[ssize] = null;
+ ascending = subit;
+ if (ascending.hasNext()) {
+ return;
+ }
+ }
+ ascending = null;
+ hasnext = false;
}
}
@@ -6,16 +6,18 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import java.util.ArrayList;
import java.util.Iterator;
import java.util.NoSuchElementException;
+import org.junit.Test;
+
import org.jdom2.Content;
import org.jdom2.Document;
import org.jdom2.Element;
+import org.jdom2.test.util.UnitTestUtil;
import org.jdom2.util.IteratorIterable;
-import org.junit.Test;
-
@SuppressWarnings("javadoc")
public class TestDescendantIterator {
private static final String[] fellowship = new String[] {
@@ -33,6 +35,21 @@
return doc.getDescendants();
}
+ private static final Element buildTestDoc() {
+ Element hobbits = new Element(fellowship[0]);
+ hobbits.addContent(new Element(fellowship[1]));
+ hobbits.addContent(new Element(fellowship[2]));
+ hobbits.addContent(new Element(fellowship[3]));
+ Element humans = new Element(fellowship[4]);
+ humans.addContent(new Element(fellowship[5]));
+ humans.addContent(new Element(fellowship[6]));
+ humans.addContent(new Element(fellowship[7]));
+ hobbits.addContent(humans);
+ // gandalf
+ hobbits.addContent(new Element(fellowship[8]));
+ return new Element("root").addContent(hobbits);
+ }
+
@Test
public void testIteration() {
Iterator<Content> it = buildIterator();
@@ -98,5 +115,58 @@ public void testRemoveOne() {
}
}
+
+
+ @Test
+ public void testRemoves() {
+ for (int i = fellowship.length - 1; i >= 0; i--) {
+ checkRemove(i);
+ }
+ }
+
+ private void checkIterator(final Iterator<Content> it, final String...values) {
+ for (int i = 0; i < values.length; i++) {
+ assertTrue(it.hasNext());
+ assertEquals(values[i], ((Element)it.next()).getName());
+ }
+ assertFalse(it.hasNext());
+ try {
+ assertTrue(null != it.next().toString());
+ fail("Should not be able to iterate off the end of the descendants.");
+ } catch (NoSuchElementException nse) {
+ // good
+ } catch (Exception e) {
+ fail("Expected NoSuchElementException, but got " + e.getClass().getName());
+ }
+ }
+
+ private void checkRemove(final int remove) {
+ Element doc = buildTestDoc();
+ checkIterator(doc.getDescendants(), fellowship);
+ Iterator<Content> it1 = doc.getDescendants();
+ it1.next();
+ for (int i = 0; i < remove; i++) {
+ it1.next();
+ }
+ it1.remove();
+ try {
+ it1.remove();
+ fail ("Should not be able to double-remove");
+ } catch (Exception e) {
+ UnitTestUtil.checkException(IllegalStateException.class, e);
+ }
+ ArrayList<String> al = new ArrayList<String>();
+ Iterator<Content> it2 = doc.getDescendants();
+ int i = remove;
+ while (--i >= 0) {
+ it2.next();
+ }
+ while (it2.hasNext()) {
+ al.add(((Element)it2.next()).getName());
+ }
+ checkIterator(it1, al.toArray(new String[al.size()]));
+ }
+
+
}

0 comments on commit 73a7138

Please sign in to comment.