From be73eff32688adc082be4ef215daeca40576d699 Mon Sep 17 00:00:00 2001 From: Davy Landman Date: Sat, 27 Jan 2024 09:56:28 +0100 Subject: [PATCH] Solve perf issue serializing large collections We have a stackless structured visitor that doesn't require large call stacks to deal with deeply nested ASTs. While this worked great for deep ASTs, it doesn't work great for big flat collections (aka wide). Big collections would require the same amount of memory, just to prepare the stack with all the entries. Now we have iterating entries on the stack, that can be "returned to" multiple times. --- .../util/StacklessStructuredVisitor.java | 146 +++++++++++++----- .../ReferenceStructuredIValueVisitor.java | 23 +-- 2 files changed, 111 insertions(+), 58 deletions(-) diff --git a/src/main/java/io/usethesource/vallang/io/binary/util/StacklessStructuredVisitor.java b/src/main/java/io/usethesource/vallang/io/binary/util/StacklessStructuredVisitor.java index cba8996a3..97b2c9d66 100644 --- a/src/main/java/io/usethesource/vallang/io/binary/util/StacklessStructuredVisitor.java +++ b/src/main/java/io/usethesource/vallang/io/binary/util/StacklessStructuredVisitor.java @@ -15,8 +15,10 @@ import java.util.ArrayDeque; import java.util.Deque; import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.Map.Entry; - +import java.util.function.IntFunction; +import org.checkerframework.checker.nullness.qual.Nullable; import io.usethesource.capsule.Map; import io.usethesource.vallang.IBool; import io.usethesource.vallang.IConstructor; @@ -40,40 +42,81 @@ public class StacklessStructuredVisitor { public static void accept(IValue root, StructuredIValueVisitor visit) throws E { - Deque> workList = new ArrayDeque<>(); - workList.push(new NextStep<>(root, StacklessStructuredVisitor::visitValue)); + Deque> workList = new ArrayDeque<>(); + workList.push(new SingleIValueStep<>(root, StacklessStructuredVisitor::visitValue)); while (!workList.isEmpty()) { - NextStep current = workList.pop(); - current.next.accept(current.val, workList, visit); + VisitStep current = workList.peek(); + if (current.hasSteps()) { + current.step(workList, visit); + } + else { + workList.pop(); + } } } @FunctionalInterface private interface NextStepConsumer { - void accept(IValue current, Deque> worklist, StructuredIValueVisitor visit) throws E; + void accept(IValue current, Deque> worklist, StructuredIValueVisitor visit) throws E; + } + + private static abstract class VisitStep { + final NextStepConsumer next; + VisitStep(NextStepConsumer next) { + this.next = next; + } + abstract boolean hasSteps(); + abstract void step(Deque> worklist, StructuredIValueVisitor visit) throws E; } - private final static class NextStep { + private static final class SingleIValueStep extends VisitStep { final IValue val; - final NextStepConsumer next; - public NextStep(IValue val, NextStepConsumer next) { + boolean completed = false; + public SingleIValueStep(IValue val, NextStepConsumer next) { + super(next); this.val = val; - this.next = next; + } + + @Override + boolean hasSteps() { + return !completed; + } + + @Override + void step(Deque> worklist, StructuredIValueVisitor visit) throws E { + next.accept(val, worklist, visit); + completed = true; + } + } + + private static final class IteratingSteps extends VisitStep { + final Iterator values; + public IteratingSteps(Iterator values, NextStepConsumer next) { + super(next); + this.values = values; + } + + @Override + boolean hasSteps() { + return values.hasNext(); + } + + @Override + void step(Deque> worklist, StructuredIValueVisitor visit) throws E { + next.accept(values.next(), worklist, visit); } } - private static void visitValue(IValue current, Deque> workList, StructuredIValueVisitor visit) throws E { + private static void visitValue(IValue current, Deque> workList, StructuredIValueVisitor visit) throws E { current.accept(new IValueVisitor() { @Override public Void visitList(IList lst) throws E { if (visit.enterList(lst, lst.length())) { - workList.push(new NextStep<>(lst, (l, w, v) -> { + workList.push(new SingleIValueStep<>(lst, (l, w, v) -> { v.leaveList(l); })); - for (int i = lst.length() - 1; i >= 0; i--) { - workList.push(new NextStep<>(lst.get(i), StacklessStructuredVisitor::visitValue)); - } + workList.push(new IteratingSteps<>(lst.iterator(), StacklessStructuredVisitor::visitValue)); } return null; } @@ -82,12 +125,10 @@ public Void visitList(IList lst) throws E { @Override public Void visitSet(ISet set) throws E { if (visit.enterSet(set, set.size())) { - workList.push(new NextStep<>(set, (l, w, v) -> { + workList.push(new SingleIValueStep<>(set, (l, w, v) -> { v.leaveSet(l); })); - for (IValue v: set) { - workList.push(new NextStep<>(v, StacklessStructuredVisitor::visitValue)); - } + workList.push(new IteratingSteps<>(set.iterator(), StacklessStructuredVisitor::visitValue)); } return null; } @@ -95,14 +136,38 @@ public Void visitSet(ISet set) throws E { @Override public Void visitMap(IMap map) throws E { if (visit.enterMap(map, map.size())) { - workList.push(new NextStep<>(map, (l, w, v) -> { + workList.push(new SingleIValueStep<>(map, (l, w, v) -> { v.leaveMap(l); })); + + + // we have to iterate, key,value pairs at a time + workList.push(new IteratingSteps<>(new Iterator() { + Iterator> entries = map.entryIterator(); + @Nullable Entry currentEntry = null; + @Override + public boolean hasNext() { + return currentEntry != null || entries.hasNext(); + } + + @Override + public IValue next() { + Entry entry = currentEntry; + if (entry != null) { + currentEntry = null; + // we already did the value, so now return the key + return entry.getValue(); + } + else if (entries.hasNext()) { + entry = currentEntry = entries.next(); + return entry.getKey(); + } + else { + throw new NoSuchElementException(); + } + } + }, StacklessStructuredVisitor::visitValue)); - for (Entry entry : (Iterable>) () -> map.entryIterator()) { - workList.push(new NextStep<>(entry.getValue(), StacklessStructuredVisitor::visitValue)); - workList.push(new NextStep<>(entry.getKey(), StacklessStructuredVisitor::visitValue)); - } } return null; } @@ -110,12 +175,11 @@ public Void visitMap(IMap map) throws E { @Override public Void visitTuple(ITuple tuple) throws E { if (visit.enterTuple(tuple, tuple.arity())) { - workList.push(new NextStep<>(tuple, (l, w, v) -> { + workList.push(new SingleIValueStep<>(tuple, (l, w, v) -> { v.leaveTuple(l); })); - for (int i = tuple.arity() - 1; i >= 0; i--) { - workList.push(new NextStep<>(tuple.get(i), StacklessStructuredVisitor::visitValue)); - } + + workList.push(new IteratingSteps<>(tuple.iterator(), StacklessStructuredVisitor::visitValue)); } return null; } @@ -124,7 +188,7 @@ public Void visitTuple(ITuple tuple) throws E { public Void visitNode(INode node) throws E { // WARNING, cloned to visitConstructor, fix bugs there as well! if (visit.enterNode(node, node.arity())) { - workList.push(new NextStep<>(node, (l, w, v) -> { + workList.push(new SingleIValueStep<>(node, (l, w, v) -> { v.leaveNode(l); })); if(node.mayHaveKeywordParameters()){ @@ -134,23 +198,22 @@ public Void visitNode(INode node) throws E { @SuppressWarnings("unchecked") AbstractDefaultWithKeywordParameters nodeKw = (AbstractDefaultWithKeywordParameters)(withKW); pushKWPairs(node, nodeKw.internalGetParameters()); - workList.push(new NextStep<>(node, (l, w, v) -> { + workList.push(new SingleIValueStep<>(node, (l, w, v) -> { v.enterNodeKeywordParameters(); })); } } + + workList.push(new IteratingSteps<>(node.iterator(), StacklessStructuredVisitor::visitValue)); - for(int i = node.arity() - 1; i >= 0; i--){ - workList.push(new NextStep<>(node.get(i), StacklessStructuredVisitor::visitValue)); - } } return null; } private void pushKWPairs(IValue root, Map.Immutable namedValues) { - workList.push(new NextStep<>(root, (l,w,v) -> { + workList.push(new SingleIValueStep<>(root, (l,w,v) -> { v.leaveNamedValue(); })); @@ -159,12 +222,12 @@ private void pushKWPairs(IValue root, Map.Immutable namedValues) Iterator> entryIterator = namedValues.entryIterator(); while (entryIterator.hasNext()) { Entry param = entryIterator.next(); - workList.push(new NextStep<>(param.getValue(), StacklessStructuredVisitor::visitValue)); + workList.push(new SingleIValueStep<>(param.getValue(), StacklessStructuredVisitor::visitValue)); i--; names[i] = param.getKey(); } assert i == 0; - workList.push(new NextStep<>(root, (l,w,v) -> { + workList.push(new SingleIValueStep<>(root, (l,w,v) -> { v.enterNamedValues(names, names.length); })); } @@ -173,7 +236,7 @@ private void pushKWPairs(IValue root, Map.Immutable namedValues) public Void visitConstructor(IConstructor constr) throws E { // WARNING, cloned from visitNode, fix bugs there as well! if (visit.enterConstructor(constr, constr.arity())) { - workList.push(new NextStep<>(constr, (l, w, v) -> { + workList.push(new SingleIValueStep<>(constr, (l, w, v) -> { v.leaveConstructor(l); })); if(constr.mayHaveKeywordParameters()){ @@ -183,16 +246,14 @@ public Void visitConstructor(IConstructor constr) throws E { @SuppressWarnings("unchecked") AbstractDefaultWithKeywordParameters constrKw = (AbstractDefaultWithKeywordParameters)(withKW); pushKWPairs(constr, constrKw.internalGetParameters()); - workList.push(new NextStep<>(constr, (l, w, v) -> { + workList.push(new SingleIValueStep<>(constr, (l, w, v) -> { v.enterConstructorKeywordParameters(); })); } } - - for(int i = constr.arity() - 1; i >= 0; i--){ - workList.push(new NextStep<>(constr.get(i), StacklessStructuredVisitor::visitValue)); - } + + workList.push(new IteratingSteps<>(constr.iterator(), StacklessStructuredVisitor::visitValue)); } return null; } @@ -244,4 +305,5 @@ public Void visitRational(IRational o) throws E { } }); } + } diff --git a/src/test/java/io/usethesource/vallang/io/reference/ReferenceStructuredIValueVisitor.java b/src/test/java/io/usethesource/vallang/io/reference/ReferenceStructuredIValueVisitor.java index 7883de55a..6bcb8039f 100644 --- a/src/test/java/io/usethesource/vallang/io/reference/ReferenceStructuredIValueVisitor.java +++ b/src/test/java/io/usethesource/vallang/io/reference/ReferenceStructuredIValueVisitor.java @@ -75,13 +75,8 @@ public Void visitList(IList o) throws E { @Override public Void visitSet(ISet o) throws E { if (visit.enterSet(o, o.size())) { - List reversedSet = new ArrayList<>(); - for (IValue v: o) { - reversedSet.add(v); - } - ListIterator li = reversedSet.listIterator(reversedSet.size()); - while (li.hasPrevious()) { - li.previous().accept(this); + for (IValue v: o) { + v.accept(this); } visit.leaveSet(o); } @@ -91,15 +86,11 @@ public Void visitSet(ISet o) throws E { @Override public Void visitMap(IMap o) throws E { if (visit.enterMap(o, o.size())) { - List reversedMap = new ArrayList<>(); - for (IValue v: o) { - reversedMap.add(v); - } - ListIterator li = reversedMap.listIterator(reversedMap.size()); - while (li.hasPrevious()) { - IValue k = li.previous(); - k.accept(this); - o.get(k).accept(this); + var entries = o.entryIterator(); + while (entries.hasNext()) { + var entry = entries.next(); + entry.getKey().accept(this); + entry.getValue().accept(this); } visit.leaveMap(o); }