ESQL: Aggressively free topn by nik9000 · Pull Request #140126 · elastic/elasticsearch

nik9000 · 2026-01-02T16:56:17Z

Without this we free the entire TopNOperator and then pitch the reference to it. It'd be best if these two things were atomically locked together, but so long as everything we free is small-ish we can live with keeping the reference for a bit. We reserve some room to maneuver here. But if the thing we're freeing is quite large this ain't great. We think we have room to allocate but we don't.

With this we pitch the reference to each row as we go so they can be GCed immediately. This keeps the untracked-but-hard-referenced state to a single row at a time.

Without this we free the entire `TopNOperator` and then pitch the reference to it. It'd be best if these two things were atomically locked together, but so long as everything we free is small-ish we can live with keeping the reference for a bit. We reserve some room to maneuver here. But if the thing we're freeing is quite large this ain't great. We think we have room to allocate but we don't. With this we pitch the reference to each row as we go so they can be GCed immediately. This keeps the untracked-but-hard-referenced state to a single row at a time.

elasticsearchmachine · 2026-01-02T16:56:41Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

ivancea

LGTM!

ivancea · 2026-01-02T17:42:53Z

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNOperator.java

+                 * the circuit breaker itself. With this we're still racing, but we're only racing a
+                 * single row at a time. And single rows can only be so large. And we have enough slop


we're only racing a single row at a time

WDYM there? Would it be enough too swap the close() and "= null" statements here?

No, the row is still referenced on the current thread. It's a shame, but I think it's somewhat inherent in having a close clear the breaker. If we swapped it so we had a ramBytesUsed() method on Row then we could do what you are saying. But that doesn't seem worth it at the moment because we'd have to rewrite a bunch of tracking bits.

ivancea · 2026-01-02T17:46:32Z

...gin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNOomRaceTests.java

+            })));
+        }
+        try {
+            OperatorTestCase.runDriver(drivers);


Shouldn't this fail if the breaker doesn't trip? I suppose an OOM should happen otherwise, but I would fail() just to be sure the test is right, and remains right (E.g. No optimizations breaking)

These are tricky. If, say, this runs in a single thread and each Driver finishes before the others wake up, then it won't OOM.

I would try and take more "personal" control of the Drivers and run a couple of single-loop-iterations so I could be sure.

I ran it in a loop for many hours and it kept failing. So I'll update it.

Worst case, it fails in CI, and we revert it.
A similar test that expected a CB 100% was muted for aggs, one I added for reduce phase, and I'm considering having it lenient like this one was initially

elasticsearchmachine · 2026-01-02T19:39:25Z

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

…l_topn_free_faster

dnhatn

LGTM, thanks Nik!

GalLalouche · 2026-01-05T11:41:47Z

.../plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/topn/TopNOperator.java

+                 */
+                () -> {
+                    for (int i = 0; i < getHeapArray().length; i++) {
+                        Row row = ((Row) getHeapArray()[i]);


Nit: redundant parentheses.

GalLalouche · 2026-01-05T11:43:51Z

...gin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNOomRaceTests.java

+        int approxSizePerFilledRow = RamUsageEstimator.NUM_BYTES_OBJECT_REF * 5 + repeats * Integer.BYTES;
+        int topCount = (int) ((double) limit.getBytes() / approxSizePerFilledRow * usedByEachThread) - 1;


Extract 5 to a constant (assuming it's the "same" 5 as the one in parameters).

It's not. I'll push more comments.

GalLalouche · 2026-01-05T11:46:58Z

...gin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNOomRaceTests.java

+                blockFactory,
+                breaker,
+                topCount,
+                IntStream.range(0, repeats).mapToObj(i -> ElementType.INT).toList(),


Use Collections.nCopies instead (here and below).

That's shorter!

GalLalouche · 2026-01-05T11:52:30Z

...gin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNOomRaceTests.java

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+
+/**


While I get the setup of this test, I'm not sure what is it actually trying to test... what are we trying to verify here? That we properly break the circuits?

That we don't oom. I'll push a bigger comment explaining. But without the change that I made to the close code the test does OOM.

Not 100% of the time though. Maybe, 10%. Which is why I've got it running 20 times.

GalLalouche · 2026-01-05T11:56:00Z

.../plugin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNRowTests.java

    }

+    public void testFromHeapDump1() {
+        TopNOperator.Row row = new TopNOperator.Row(new NoopCircuitBreaker(CircuitBreaker.REQUEST), sortOrders(5), 56, 24);


The relationship between 5, 56, and 24 and 304L isn't obvious to me (same thing for the numbers in the method below). Are they just magic numbers, do they come from the same heap dump or the 304L, or is it something else?

They come from the heap dump. They are actual measured results. I'll leave a comment.

GalLalouche · 2026-01-05T11:56:17Z

.../plugin/esql/compute/src/test/java/org/elasticsearch/compute/operator/topn/TopNRowTests.java

+    }
+
+    public void testFromHeapDump2() {
+        TopNOperator.Row row = new TopNOperator.Row(new NoopCircuitBreaker(CircuitBreaker.REQUEST), sortOrders(1), 1160, 1153096);


s/1153096/1_153_096

nik9000 · 2026-01-05T14:10:59Z

@GalLalouche, I pushed a patch for your comments. Could you have another look?

…l_topn_free_faster

Without this we free the entire `TopNOperator` and then pitch the reference to it. It'd be best if these two things were atomically locked together, but so long as everything we free is small-ish we can live with keeping the reference for a bit. We reserve some room to maneuver here. But if the thing we're freeing is quite large this ain't great. We think we have room to allocate but we don't. With this we pitch the reference to each row as we go so they can be GCed immediately. This keeps the untracked-but-hard-referenced state to a single row at a time.

elasticsearchmachine · 2026-01-05T16:17:01Z

💚 Backport successful

Status	Branch	Result
✅	9.3

Without this we free the entire `TopNOperator` and then pitch the reference to it. It'd be best if these two things were atomically locked together, but so long as everything we free is small-ish we can live with keeping the reference for a bit. We reserve some room to maneuver here. But if the thing we're freeing is quite large this ain't great. We think we have room to allocate but we don't. With this we pitch the reference to each row as we go so they can be GCed immediately. This keeps the untracked-but-hard-referenced state to a single row at a time.

nik9000 added auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.3.0 v9.4.0 labels Jan 2, 2026

nik9000 requested review from GalLalouche, dnhatn and ivancea January 2, 2026 16:56

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 2, 2026

ivancea approved these changes Jan 2, 2026

View reviewed changes

nik9000 added the >bug label Jan 2, 2026

Update docs/changelog/140126.yaml

be31173

nik9000 added 2 commits January 2, 2026 18:06

Force

eddcf32

Merge remote-tracking branch 'nik9000/esql_topn_free_faster' into esq…

10137e2

…l_topn_free_faster

nik9000 enabled auto-merge (squash) January 2, 2026 23:07

dnhatn approved these changes Jan 5, 2026

View reviewed changes

GalLalouche reviewed Jan 5, 2026

View reviewed changes

GalLalouche requested changes Jan 5, 2026

View reviewed changes

ivancea approved these changes Jan 5, 2026

View reviewed changes

Updates

33832b9

nik9000 requested a review from GalLalouche January 5, 2026 14:10

GalLalouche approved these changes Jan 5, 2026

View reviewed changes

nik9000 added 4 commits January 5, 2026 09:42

Merge branch 'main' into esql_topn_free_faster

5ee755e

Extra

abd7d38

Merge remote-tracking branch 'nik9000/esql_topn_free_faster' into esq…

5a72bd2

…l_topn_free_faster

Merge branch 'main' into esql_topn_free_faster

d779aa8

nik9000 merged commit d6762fb into elastic:main Jan 5, 2026
35 checks passed

nik9000 mentioned this pull request Jan 5, 2026

[9.3] ESQL: Aggressively free topn (#140126) #140174

Merged

alex-spies mentioned this pull request Jan 7, 2026

[CI] TopNOomRaceTests.testRace failed #140258

Open

This was referenced Jan 13, 2026

[CLEAN] Synthetic Benchmark PR #140126 - ESQL: Aggressively free topn qodo-benchmark/elasticsearch#65

Open

[CORRUPTED] Synthetic Benchmark PR #140126 - ESQL: Aggressively free topn qodo-benchmark/elasticsearch#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Aggressively free topn#140126

ESQL: Aggressively free topn#140126
nik9000 merged 9 commits intoelastic:mainfrom
nik9000:esql_topn_free_faster

nik9000 commented Jan 2, 2026

elasticsearchmachine commented Jan 2, 2026

ivancea left a comment

ivancea Jan 2, 2026

nik9000 Jan 2, 2026

ivancea Jan 2, 2026

nik9000 Jan 2, 2026

nik9000 Jan 2, 2026

ivancea Jan 5, 2026

elasticsearchmachine commented Jan 2, 2026

dnhatn left a comment

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

nik9000 Jan 5, 2026

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

GalLalouche Jan 5, 2026

nik9000 Jan 5, 2026

nik9000 commented Jan 5, 2026

Uh oh!

elasticsearchmachine commented Jan 5, 2026

Labels

5 participants

		* the circuit breaker itself. With this we're still racing, but we're only racing a
		* single row at a time. And single rows can only be so large. And we have enough slop

		int approxSizePerFilledRow = RamUsageEstimator.NUM_BYTES_OBJECT_REF * 5 + repeats * Integer.BYTES;
		int topCount = (int) ((double) limit.getBytes() / approxSizePerFilledRow * usedByEachThread) - 1;

Conversation

nik9000 commented Jan 2, 2026

elasticsearchmachine commented Jan 2, 2026

ivancea left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 2, 2026

dnhatn left a 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

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

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

nik9000 commented Jan 5, 2026

Uh oh!

elasticsearchmachine commented Jan 5, 2026

💚 Backport successful

Labels

5 participants