Attempt to make some operations cheaper #125228

idegtiarenko · 2025-03-19T14:54:54Z

This attempts to make optimizing a tiny bit cheaper.
Please see inline comments for details.

Related to: #124395

idegtiarenko · 2025-03-19T14:57:04Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java

+        // to avoid allocating iterator that performs concurrent modification checks
+        for (int c = 0; c < children.size(); c++) {
+            children.get(c).forEachUp(action);
+        }


forEachDown and forEachUp do not seem to have visible impact on profile I reviewed, however replacing them to loops should make profiles outputs several stacks shorter, especially when those are nested.

Could we add this as a comment, please? The explanation in the comment helps, but I think the important point is that this reduces stack size rather than increasing performance, which the current comment kinda implies by mentioning the concurrent modification checks - which I understand didn't seem to be a problem?

idegtiarenko · 2025-03-19T14:58:52Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

+            // please do not refactor it to a for-each loop
+            // to avoid allocating iterator that performs concurrent modification checks
+            for (int i = 0; i < c.size(); i++) {
+                var e = c.get(i);


ArrayList.Itr is surprisingly visible in profile (those are blue rectangles on top left of the pink ones) when executing this code (maybe because it spends time checking for concurrent modifications?).
Replacing it with a for loop should make it a bit cheaper.

We're making more and more improvements to how we perform query plan transformations/traversals.

Could we please add at least a micro benchmark as one of the next steps? Without one, it's both hard to understand the impact of optimizations, and it's also unclear if we maybe introduce accidental regressions (in whichever PRs).

Good point. I will try to add one in a separate change.

idegtiarenko · 2025-03-19T14:59:12Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

        // has no type info so it's difficult to have automatic checking without having base classes).

-        if (arg instanceof Collection<?> c) {
+        if (arg instanceof List<?> c) {


Please confirm we do not expect sets in here.

We don't however it's not enforced - add an else throwing an error for safety.

++ this could easily become a bug in the future, as it makes total sense for a node property to be e.g. a set of expressions. We might also just not have any test that exercises this code path with a non-list collection. To really know, we'd have to go and check all the info() method implementations on Node subclasses manually. And enforce that the only collection that node properties can use are lists.

My suggestion: let's add a code path for List<?> in addition to the more general code path for Collection<?> - and let's add an assertion in the latter because that code path shouldn't be used, so that this only trips in tests but doesn't damage production code.

costin · 2025-03-20T01:14:29Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java

-        children().forEach(c -> c.forEachDown(action));
+        // please do not refactor it to a for-each loop
+        // to avoid allocating iterator that performs concurrent modification checks
+        for (int c = 0; c < children.size(); c++) {


Let's squeeze a bit more perf:

for (int c = 0, s = children.size(); c < s; c++)

costin

LGTM.
Please keep an eye out of stream usage - I've specifically avoided them in the core however they are used in some rules and they started showing up last I checked the profiler.

costin · 2025-03-20T01:15:29Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

        // has no type info so it's difficult to have automatic checking without having base classes).

-        if (arg instanceof Collection<?> c) {
+        if (arg instanceof List<?> c) {


We don't however it's not enforced - add an else throwing an error for safety.

costin · 2025-03-20T01:18:18Z

Being a small change, it makes sense to backport this all the way to 8.x, especially after seeing the list iterator bubbling up.

elasticsearchmachine · 2025-03-20T07:16:58Z

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

alex-spies

Thanks Ievgen, this generally LGTM. However, I think we should preserve the Collection code path in QueryPlan.doTransformExpression just in case that some node property is not a list. Alternatively, we could also double check and enforce that node properties that are collections can only be lists, and place that enforcement close to the actual node properties so it's possible to validate this invariant without relying on (potentially insufficient) testing.

A thought: the improvement to Node.java works because the children of a query plan are not mutable. I wonder if we could make this clearer/more idiomatic by turning the children into an unmodifiable list - which would also throw exceptions in case we accidentally don't respect this invariant somewhere.

alex-spies · 2025-03-20T10:03:14Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java

+        // to avoid allocating iterator that performs concurrent modification checks
+        for (int c = 0; c < children.size(); c++) {
+            children.get(c).forEachUp(action);
+        }


Could we add this as a comment, please? The explanation in the comment helps, but I think the important point is that this reduces stack size rather than increasing performance, which the current comment kinda implies by mentioning the concurrent modification checks - which I understand didn't seem to be a problem?

alex-spies · 2025-03-20T10:15:23Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

+            // please do not refactor it to a for-each loop
+            // to avoid allocating iterator that performs concurrent modification checks
+            for (int i = 0; i < c.size(); i++) {
+                var e = c.get(i);


We're making more and more improvements to how we perform query plan transformations/traversals.

Could we please add at least a micro benchmark as one of the next steps? Without one, it's both hard to understand the impact of optimizations, and it's also unclear if we maybe introduce accidental regressions (in whichever PRs).

alex-spies · 2025-03-20T10:23:06Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

        // has no type info so it's difficult to have automatic checking without having base classes).

-        if (arg instanceof Collection<?> c) {
+        if (arg instanceof List<?> c) {


++ this could easily become a bug in the future, as it makes total sense for a node property to be e.g. a set of expressions. We might also just not have any test that exercises this code path with a non-list collection. To really know, we'd have to go and check all the info() method implementations on Node subclasses manually. And enforce that the only collection that node properties can use are lists.

My suggestion: let's add a code path for List<?> in addition to the more general code path for Collection<?> - and let's add an assertion in the latter because that code path shouldn't be used, so that this only trips in tests but doesn't damage production code.

alex-spies · 2025-03-20T10:27:57Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java


            return hasChanged ? transformed : arg;
        }
+        assert arg instanceof Set<?> == false : "Set arguments are not supported";


This is insufficient in theory - someone in the future could use a Queue (for whatever reason) or a custom implementation of Collection. A safer way is to just assert false here in case of Collection and saying that non-list collections are not allowed.

Good point. Let me change this to Collection<?>.
I would like to avoid another branch with custom logic for collection for now as somebody might accidentally use it without knowing its performance limitation.
We can add it when we see the set or any other custom implementation is absolutely required.

As discussed offline: in practical terms, yeah that'd likely be fine. But I'm also paranoid as this is called on general node properties and for whatever reason there might just be one node subclass that never hit this code path in tests, yet.

We discussed this in person and decided to keep the other branch for now (in case this behaviour is actually used but not covered by tests).

alex-spies

Thanks Ievgen, LGTM!

alex-spies · 2025-03-20T12:41:12Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java

        action.accept((T) this);
-        children().forEach(c -> c.forEachDown(action));
+        // please do not refactor it to a for-each loop to avoid
+        // allocating iterator that performs concurrent modification checks and extra stack traces


nit:

Suggested change

// allocating iterator that performs concurrent modification checks and extra stack traces

// allocating iterator that performs concurrent modification checks and extra stack frames

alex-spies · 2025-03-20T12:43:05Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

+            }
+            return hasChanged ? transformed : arg;
+        } else if (arg instanceof Collection<?> c) {
            List<Object> transformed = null;


I think it's fine to mark this branch as deprecated/likely dead + we could still add an assert false here - before maybe later getting rid of this branch altogether and enforcing that the only collections used as properties are all lists (but in a more visible place, e.g. in the c'tor of QueryPlan/Node).

elasticsearchmachine · 2025-03-21T08:10:20Z

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	8.x

Attempt to make some operations cheaper

16a15af

idegtiarenko added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.1.0 labels Mar 19, 2025

idegtiarenko requested a review from costin March 19, 2025 14:54

github-actions bot deployed to docs-preview March 19, 2025 14:55 View deployment

idegtiarenko commented Mar 19, 2025

View reviewed changes

costin reviewed Mar 20, 2025

View reviewed changes

costin approved these changes Mar 20, 2025

View reviewed changes

idegtiarenko marked this pull request as ready for review March 20, 2025 07:16

idegtiarenko added auto-backport Automatically create backport pull requests when merged v8.19.0 labels Mar 20, 2025

idegtiarenko added 2 commits March 20, 2025 09:20

upd

6250cde

Merge branch 'main' into 124395

330ba8f

idegtiarenko added the v9.0.0 label Mar 20, 2025

alex-spies requested changes Mar 20, 2025

View reviewed changes

upd

634bc38

idegtiarenko requested a review from alex-spies March 20, 2025 12:11

idegtiarenko and others added 3 commits March 20, 2025 13:11

Merge branch 'main' into 124395

c13268c

[CI] Auto commit changes from spotless

94b1f41

upd

86b0f8b

alex-spies approved these changes Mar 20, 2025

View reviewed changes

idegtiarenko added 2 commits March 20, 2025 14:07

fix typo

4eb0220

Merge branch 'main' into 124395

3e11d25

idegtiarenko merged commit 9ddcede into elastic:main Mar 21, 2025
17 checks passed

This was referenced Mar 21, 2025

[9.0] Attempt to make some operations cheaper (#125228) #125362

Merged

[8.x] Attempt to make some operations cheaper (#125228) #125363

Merged

idegtiarenko added a commit to idegtiarenko/elasticsearch that referenced this pull request Mar 21, 2025

Attempt to make some operations cheaper (elastic#125228)

71bf7e5

idegtiarenko deleted the 124395 branch March 21, 2025 08:23

elasticsearchmachine pushed a commit that referenced this pull request Mar 21, 2025

Attempt to make some operations cheaper (#125228) (#125363)

861b7dc

elasticsearchmachine pushed a commit that referenced this pull request Mar 21, 2025

Attempt to make some operations cheaper (#125228) (#125362)

69ae708

afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Mar 21, 2025

Attempt to make some operations cheaper (elastic#125228)

4ca23c5

smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025

Attempt to make some operations cheaper (elastic#125228)

baa624f

omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025

Attempt to make some operations cheaper (elastic#125228)

8c6e485

	// allocating iterator that performs concurrent modification checks and extra stack traces
	// allocating iterator that performs concurrent modification checks and extra stack frames

Attempt to make some operations cheaper #125228

Attempt to make some operations cheaper #125228

Uh oh!

Conversation

idegtiarenko commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idegtiarenko Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 20, 2025

Uh oh!

alex-spies left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 21, 2025

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

idegtiarenko Mar 19, 2025 •

edited

Loading

costin commented Mar 20, 2025 •

edited

Loading

alex-spies left a comment •

edited

Loading