Reduce unnecessary memory allocations in Cascades #2464

hatyo · 2024-01-24T11:29:53Z

This is mainly refactoring to improve memory allocation in Cascades.

Value#getChildren() is now amortized. Previously, many implementations were calculating the list of children each time the method is invoked, but this is not necessary considering the immutability of Value.
TreeLike#inPreOrder(Predicate<>) is slightly incorrect since the predicate does not propagate in recursive call, but more importantly, it is inefficient in terms of memory consumption, considering that it creates a list for each node of the tree each time it is invoked, the lists are accessed via a complex hierarchy of concatenated iterators. These small object allocations are unnecessary and could stress the garbage collection if it is done very frequently which is the case since this method is used indirectly e.g. to understand a Predicate structure during index matching.
In this PR a new implementation that is more efficient is introduced. It employs an algorithm that mixes level-based and preorder-based traversal. It uses a stack which holds a reference to the entire list of children and an index to bookkeep the traversed node. It is exposed as a normal Iterator<>.
With the new Iterator, the TreeLike now extends Iterable<> interface giving an instance of this iterator when iterator() method is called.
Similar to Collection.stream(), a stream() method is added that returns a Stream object which uses the above Iterator implementation.
I noticed that in almost all callsites, TreeLike#inPreOrder's allocated Iterable is not used, i.e. it is mostly used for stream-like interface filtering, therefore, replacing these calls with the stream() call implemented above is probably more efficient as it does not allocate as many objects. It also streamlines the code better.
a height() method is introduced in TreeLike that returns the max-height of the tree, the implementations of this method are amortized, it is mainly used to set the capacity of the pre-order iterator stack so we can avoid yet other potential reallocations needed for resizing.

I kept the inPreOrder overloads, and filter but marked them as deprecated in favour of using stream() (and Stream#filter()). But I am totally fine if we want to remove them immediately.

To compare the new implementation of pre-order with the older one, I implemented a simple JMH Benchmark in a separate draft PR #2467, please check it out if you want to get more performance details.

hatyo · 2024-01-25T19:31:10Z

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/PlanHashable.java

+        if (obj instanceof PlanHashable) {
+            return ((PlanHashable)obj).planHash(mode);
+        }


Changing the order here is necessary, because now a Value is actually an Iterable, therefore, it will match line 250.

If we do not change the order of lines, the following will happen:

a Value such as RCV(x,y,z) will match at line 250, calls into iterablePlanHash.

the method iterablePlanHash will iterate over the passed iterable object

the first item in the RCV(x,y,z) is the RCV itself (because of pre-order and the recursive nature of the non-linear structure TreeLike<>.

it calls now in objectPlanHash

it goes back to 1 (!)

-> infinite recursion.

Is this keeping existing plan hashes stable?

I am assuming the plan hash has enough test coverage such that if the PRB is green (which is), then plan hash should not have been changed.

Restored the code now that TreeLike does not extend Iterable.

sonarcloud · 2024-01-25T20:09:59Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

11 New issues
0 Security Hotspots
90.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

foundationdb-ci · 2024-01-25T20:11:05Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: a1e000b
Duration 0:44:20
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

normen662 · 2024-01-29T19:48:49Z

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/PlanHashable.java

+        if (obj instanceof PlanHashable) {
+            return ((PlanHashable)obj).planHash(mode);
+        }


Is this keeping existing plan hashes stable?

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/TreeLike.java

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/NotValue.java

normen662 · 2024-01-29T20:02:03Z

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/TreeLike.java

     */
    @Nonnull
+    @Deprecated


Please have this use your much better iterator.

Method is replaced with a normal Stream#filter as now TreeLike can return a Stream via TreeLike#preOrderStream() method.

...re/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/AbstractValue.java

normen662 · 2024-01-29T20:04:17Z

...-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/AndOrValue.java

@@ -68,6 +70,8 @@ public class AndOrValue extends AbstractValue implements BooleanValue {
    @Nonnull
    private final Value rightChild;
    @Nonnull
+    private final Supplier<Iterable<? extends Value>> childrenSupplier = Suppliers.memoize(this::computeChildren);


Again, please condense into AbstractValue

Done, I also refactored ValueWithChild implementations to avoid extra allocation.

normen662 · 2024-01-29T20:08:44Z

Hey, I like the iterator a lot, it's much more performant. Also, quite a bit of resolve to properly profile it. I have not looked into the replace() changes yet as I hit submit too early. Will do that tomorrow.

foundationdb-ci · 2024-02-01T16:11:23Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: a1e000b
Duration 0:42:11
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-02-01T16:22:57Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 9d65c0a
Duration 0:22:45
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-02-01T16:25:58Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: b1837e9
Duration 0:24:11
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

…_like

foundationdb-ci · 2024-02-02T10:33:16Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 83aa17f
Duration 0:42:10
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-02-05T08:03:07Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 0a0d74f
Duration 0:41:03
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/TreeLike.java

normen662 · 2024-02-05T09:07:43Z

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/TreeLike.java

@@ -252,6 +255,8 @@ default Optional<T> replaceLeavesMaybe(@Nonnull final UnaryOperator<T> replaceOp
        });
    }

+    int height();


Can't we just implement height() as a static TreeLike method() that this method calls. That uses a fold() as there is nothing specific that height() needs from Value or QueryPredicate or such. If you then need to memoize that you can by defining a height() method in the particular AbstractValue or similar with a memoizing supplier.

Done. Check my reply above.

normen662 · 2024-02-05T08:55:16Z

...r-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/PreOrderIterator.java

+public final class PreOrderIterator<T extends TreeLike<T>> implements Iterator<T> {
+
+    @Nonnull
+    private final Deque<Pair<Iterable<? extends T>, Integer>> stack;


Can you not use Pair but define your own class? This is due to us removing commons stuff in the future.

Right, but I don't want to do it in this PR to limit its scope, there are 80+ locations where we still use Pair currently, we can do this in a separate cleanup PR. (I am also using MutablePair by the way, but we can also replace that in a cleanup PR). #2484

...ord-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/TreeLike.java

...re/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/AbstractValue.java

normen662 · 2024-02-05T09:13:30Z

...re/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/AbstractValue.java

+    }
+
+    private int computeHeight() {
+        return StreamSupport.stream(getChildren().spliterator(), false).mapToInt(TreeLike::height).max().orElse(0) + 1;


Streams.stream(...) please.

Done. Method is refactored and sits now in TreeLike.

...core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/values/ExistsValue.java

…f interfaces.

foundationdb-ci · 2024-02-05T11:30:57Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 4b62403
Duration 0:41:25
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-02-05T13:28:12Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 8ab73b6
Duration 0:39:17
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

normen662 · 2024-02-05T18:06:09Z

...ava/com/apple/foundationdb/record/query/plan/cascades/predicates/AbstractQueryPredicate.java

@@ -56,6 +58,7 @@ protected AbstractQueryPredicate(final boolean isAtomic) {
        this.isAtomic = isAtomic;
        this.correlatedToSupplier = Suppliers.memoize(this::computeCorrelatedTo);
        this.semanticHashCodeSupplier = Suppliers.memoize(this::computeSemanticHashCode);
+        this.heightSupplier = Suppliers.memoize(QueryPredicate.super::height);


I like that.

normen662 · 2024-02-05T18:07:59Z

.../main/java/com/apple/foundationdb/record/query/plan/cascades/values/ConstantObjectValue.java

@@ -83,6 +84,12 @@ public Set<CorrelationIdentifier> getCorrelatedToWithoutChildren() {
        return Set.of();


Can you please make this a ImmutableSet.of(). You didn't change it here, I just stumbled over it.

ok, I will do as part of solving #2483

This comment was marked as outdated.

Sign in to view

hatyo mentioned this pull request Jan 25, 2024

DO NOT MERGE - Measure performance of pre-order implementations #2467

Draft

This comment was marked as outdated.

Sign in to view

hatyo changed the title ~~WIP - Reduce unnecessary memory allocations.~~ Reduce unnecessary memory allocations in Cascades Jan 25, 2024

hatyo commented Jan 25, 2024

View reviewed changes

hatyo marked this pull request as ready for review January 25, 2024 20:28

normen662 requested changes Jan 29, 2024

View reviewed changes

hatyo added 7 commits February 1, 2024 16:49

Memoize children calculation where applicable.

31262ba

TreeLike<> now extends Iterable<>

7ef3549

Fix checkstyle violation.

44daf6d

Change order of pattern matches to avoid infinite recursion.

cd928a8

Refactoring, add height() to TreeLike<> to improve Stack allocation.

3c4777b

Fix spotbugs violations.

bc017e0

Address comments.

9d65c0a

hatyo force-pushed the reduce_streams_tree_like branch from a1e000b to 9d65c0a Compare February 1, 2024 16:00

refactoring.

b1837e9

hatyo added 2 commits February 2, 2024 09:37

Merge remote-tracking branch 'upstream/main' into reduce_streams_tree…

af32934

…_like

Further refactoring.

83aa17f

Update release notes.

0a0d74f

normen662 requested changes Feb 5, 2024

View reviewed changes

Revert making LeafValue and ValueWithChild abstract classes instead o…

4b62403

…f interfaces.

Address remaining comments.

8ab73b6

normen662 reviewed Feb 5, 2024

View reviewed changes

normen662 approved these changes Feb 5, 2024

View reviewed changes

hatyo mentioned this pull request Feb 6, 2024

Use ImmutableList where applicable #2483

Open

hatyo merged commit fc4304a into FoundationDB:main Feb 6, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce unnecessary memory allocations in Cascades #2464

Reduce unnecessary memory allocations in Cascades #2464

hatyo commented Jan 24, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

hatyo Jan 25, 2024

normen662 Jan 29, 2024

hatyo Feb 1, 2024

hatyo Feb 1, 2024

sonarcloud bot commented Jan 25, 2024

foundationdb-ci commented Jan 25, 2024

normen662 Jan 29, 2024

normen662 Jan 29, 2024

hatyo Feb 5, 2024

normen662 Jan 29, 2024

hatyo Feb 1, 2024

normen662 commented Jan 29, 2024

foundationdb-ci commented Feb 1, 2024

foundationdb-ci commented Feb 1, 2024

foundationdb-ci commented Feb 1, 2024

foundationdb-ci commented Feb 2, 2024

foundationdb-ci commented Feb 5, 2024

normen662 Feb 5, 2024

hatyo Feb 5, 2024

normen662 Feb 5, 2024 •

edited

Loading

hatyo Feb 5, 2024

normen662 Feb 5, 2024

hatyo Feb 5, 2024

foundationdb-ci commented Feb 5, 2024

foundationdb-ci commented Feb 5, 2024

normen662 Feb 5, 2024

normen662 Feb 5, 2024

hatyo Feb 6, 2024

		@@ -83,6 +84,12 @@ public Set<CorrelationIdentifier> getCorrelatedToWithoutChildren() {
		return Set.of();

Reduce unnecessary memory allocations in Cascades #2464

Reduce unnecessary memory allocations in Cascades #2464

Conversation

hatyo commented Jan 24, 2024 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 25, 2024

Quality Gate passed

foundationdb-ci commented Jan 25, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normen662 commented Jan 29, 2024

foundationdb-ci commented Feb 1, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Feb 1, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Feb 1, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Feb 2, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Feb 5, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normen662 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foundationdb-ci commented Feb 5, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Feb 5, 2024

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hatyo commented Jan 24, 2024 •

edited

Loading

normen662 Feb 5, 2024 •

edited

Loading