Avoid humongous blocks #103340

dnhatn · 2023-12-12T18:07:24Z

The HeapAttackIT#testFetchTooManyMvLongs was OOM, even though plenty of memory was available and all memory usage was accurately tracked. The issue lies in the wasteful space occupied by humongous objects, specifically blocks with large backing arrays. The space left in regions by these humongous objects remains unused. This PR addresses the problem by forcing blocks to use BigArrays once the memory used by their primitive backing array exceeds a specified threshold - 512KB in this PR.

dnhatn · 2023-12-12T19:50:32Z

...esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBigArrayBlock.java

+ */
+public final class BooleanBigArrayBlock extends AbstractArrayBlock implements BooleanBlock {
+
+    private static final long BASE_RAM_BYTES_USED = 0; // TODO: fix this


I will address this in a follow-up for both BigArrays Vectors and Blocks.

dnhatn · 2023-12-12T19:52:34Z

...n/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/DoubleBlockBuilder.java

-                    );
+                    int currentLength = array.length;
+                    // TODO: should be ANDed instead?
+                    if (currentLength - valueCount > 1024 || valueCount < (currentLength / 2)) {


I think it should be ANDed instead of ORed. Let's discuss this, and I can make the change in a follow-up if we agree.

++ happy to separate it.

dnhatn · 2023-12-12T20:04:05Z

...p-attack/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/heap_attack/HeapAttackIT.java

@@ -344,7 +344,6 @@ public void testFetchMvLongs() throws IOException {
        assertMap(map, matchesMap().entry("columns", columns));
    }

-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/100528")


We can unmute this test now.

elasticsearchmachine · 2023-12-12T20:05:02Z

Hi @dnhatn, I've created a changelog YAML for you.

elasticsearchmachine · 2023-12-12T20:05:34Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-12-12T20:05:34Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

ChrisHegarty

I think that this is a good change. it would be interesting to see what impact it might have on performance, but we can check that post-merge in the nightly benchmarks, since the reliability is the higher-order bit here.

dnhatn · 2023-12-13T15:38:45Z

it would be interesting to see what impact it might have on performance, but we can check that post-merge in the nightly benchmarks, since the reliability is the higher-order bit here

Thanks Chris! Yes, that's very possible. I will keep an eye on the benchmark. If there is a performance impact, then I think we switch to BigArrays blocks only in the build method.

nik9000

Maybe we should limit the number of elements in the block rather than move to big arrays? We don't have a good way to cut existing blocks but maybe that'd be better?

nik9000 · 2023-12-13T15:52:12Z

...n/esql/compute/src/main/generated-src/org/elasticsearch/compute/data/DoubleBlockBuilder.java

-    private double[] values;
+    private interface DoubleValues extends Releasable {
+        void setValue(int index, double value);
+    }


I worry about these things slowing things down by making the write bimorphic instead of an array access.

I've pushed 5e5330d.

dnhatn · 2023-12-13T16:31:57Z

Maybe we should limit the number of elements in the block rather than move to big arrays?

Thanks, Nik. I think we prefer not to fail the query in this case and we don't have a correct estimate upfront for multi-valued fields. I think we can avoid the write bimorphic. Since the number of concurrent builders is bounded by the number of execution pipepline, it's okay to always use primitive arrays for the block builders. And in the build method, we can decide to switch to BigsArray if its size greater than the threshold. I will update this PR.

alex-spies

I only got to take a short look at this PR, but I think the approach is fine. Left a minor remark only.

...esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBigArrayBlock.java

alex-spies · 2023-12-14T10:04:49Z

Maybe we should limit the number of elements in the block rather than move to big arrays?

Thanks, Nik. I think we prefer not to fail the query in this case and we don't have a correct estimate upfront for multi-valued fields. I think we can avoid the write bimorphic. Since the number of concurrent builders is bounded by the number of execution pipepline, it's okay to always use primitive arrays for the block builders. And in the build method, we can decide to switch to BigsArray if its size greater than the threshold. I will update this PR.

Hmm, but do we really need to fail the query?

I guess, ideally we would want to emit multiple blocks once a certain total position count of the block is reached, no? (Currently, I think we do this based on position count only, which doesn't work with MVs.)

With BigArray-backed blocks, isn't there a risk that some downstream operator makes wasteful use of memory because it expects a reasonably-sized block that it can process in one go? testFetchTooManyMvLongs only tests a simple query FROM mv_longs, but I'm not sure if that really guarantees that a more complex query couldn't do something bad with humongous blocks, no?

In any case, IMO this is fine as it avoids OOM in more cases, but maybe a longer-term solution would be to cut blocks after a certain size when working with MVs.

dnhatn · 2023-12-18T15:34:04Z

Thanks, everyone! I am going to merge this PR and will keep an eye on the benchmark for the next several days.

The HeapAttackIT#testFetchTooManyMvLongs was OOM, even though plenty of memory was available and all memory usage was accurately tracked. The issue lies in the wasteful space occupied by humongous objects, specifically blocks with large backing arrays. The space left in regions by these humongous objects remains unused. This PR addresses the problem by forcing blocks to use BigArrays once the memory used by their primitive backing array exceeds a specified threshold - 512KB in this PR.

Avoid humongous blocks

25d9077

elasticsearchmachine added the v8.13.0 label Dec 12, 2023

wording

68a0835

dnhatn commented Dec 12, 2023

View reviewed changes

dnhatn added :Analytics/ES|QL AKA ESQL >enhancement labels Dec 12, 2023

dnhatn requested review from nik9000 and ChrisHegarty December 12, 2023 20:04

Update docs/changelog/103340.yaml

9cec4d8

dnhatn requested a review from alex-spies December 12, 2023 20:05

dnhatn marked this pull request as ready for review December 12, 2023 20:05

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Dec 12, 2023

ChrisHegarty approved these changes Dec 13, 2023

View reviewed changes

nik9000 reviewed Dec 13, 2023

View reviewed changes

alex-spies reviewed Dec 13, 2023

View reviewed changes

...esql/compute/src/main/generated-src/org/elasticsearch/compute/data/BooleanBigArrayBlock.java Show resolved Hide resolved

switch to BigArrays on build

5e5330d

dnhatn requested a review from nik9000 December 13, 2023 17:27

Merge remote-tracking branch 'elastic/main' into big-arrays

9b3b481

dnhatn merged commit 4a583d9 into elastic:main Dec 18, 2023
15 checks passed

dnhatn deleted the big-arrays branch December 18, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid humongous blocks #103340

Avoid humongous blocks #103340

dnhatn commented Dec 12, 2023 •

edited

dnhatn Dec 12, 2023

dnhatn Dec 12, 2023

ChrisHegarty Dec 13, 2023

dnhatn Dec 12, 2023

elasticsearchmachine commented Dec 12, 2023

elasticsearchmachine commented Dec 12, 2023

elasticsearchmachine commented Dec 12, 2023

ChrisHegarty left a comment

dnhatn commented Dec 13, 2023

nik9000 left a comment

nik9000 Dec 13, 2023

dnhatn Dec 13, 2023

dnhatn commented Dec 13, 2023

alex-spies left a comment

alex-spies commented Dec 14, 2023

dnhatn commented Dec 18, 2023

Avoid humongous blocks #103340

Avoid humongous blocks #103340

Conversation

dnhatn commented Dec 12, 2023 • edited

dnhatn Dec 12, 2023

Choose a reason for hiding this comment

dnhatn Dec 12, 2023

Choose a reason for hiding this comment

ChrisHegarty Dec 13, 2023

Choose a reason for hiding this comment

dnhatn Dec 12, 2023

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 12, 2023

elasticsearchmachine commented Dec 12, 2023

elasticsearchmachine commented Dec 12, 2023

ChrisHegarty left a comment

Choose a reason for hiding this comment

dnhatn commented Dec 13, 2023

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Dec 13, 2023

Choose a reason for hiding this comment

dnhatn Dec 13, 2023

Choose a reason for hiding this comment

dnhatn commented Dec 13, 2023

alex-spies left a comment

Choose a reason for hiding this comment

alex-spies commented Dec 14, 2023

dnhatn commented Dec 18, 2023

dnhatn commented Dec 12, 2023 •

edited