Fix concurrency issue in ScriptSortBuilder #123757

javanna · 2025-02-28T19:31:19Z

Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request.

The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc.

The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause.

elasticsearchmachine · 2025-03-05T23:02:50Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

elasticsearchmachine · 2025-03-05T23:02:51Z

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

javanna · 2025-03-05T23:04:36Z

modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/30_search.yml

+  - match: { aggregations.test.buckets.1.key: "second" }
+  - match: { aggregations.test.buckets.1.top_hits.hits.total: 2 }
+  - match: { aggregations.test.buckets.1.top_hits.hits.hits.0.sort.0: "d-1.0" }
+  - match: { aggregations.test.buckets.1.top_hits.hits.hits.0.sort.1: 1.0 }


it appears that we had no tests covering sort scripts relying on _score. I added them because making _score available in sort scripts was the whole reason why script sorting wasn't thread-safe when inter-segment concurrency is enabled.

javanna · 2025-03-05T23:05:31Z

...er/src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java

+                }
+            }
+        );
+    }


This test was also missing and if present, would have surfaced concurrency issues when using script sorting as part as top_hits.

I'm trying to see this test fail without the changes to ScriptSortBuilder and the two field comparators but I could not get it to fail so far, even with several hundred of iterations and several re-runs with a different root seed. I see the index contains 50 docs, how are we sure its likely to run into using several segments / concurrent situations? I remember some tweaks to our integration tests to make this more likely for small indices but cannot remember, do you recall?

are you sure that you reverted the changes to ScriptSortBuilder in the right places, still leaving concurrency enabled for script sorting? I get it to fail as I soon as I do that. There is maybe a little less likelihood for it to fail due to randomizing the execution hint, because we parallelize only on global_ordinals execution mode, based on field cardinality. That is for the terms aggs that holds the top_hits agg. But I find that the issue reproduces pretty quickly.

Yes, I reverted all changes that are not test-related (tried also with and wothough changes to MockScriptEngine), and ran the test very often from IDE and from command line, no failures so far. I'll try running in a tight loop once again

you need to keep overriding supportsParallelCollection in ScriptSortBuilder :) otherwise it does not run using inter-segment concurrency.

original-brownbear · 2025-03-06T10:58:40Z

Looks like this fails with a legitimate and related failure: https://buildkite.com/elastic/elasticsearch-pull-request/builds/60890#01956897-cd53-49cc-9f12-67c7ab7ceef1/93-5575

cbuescher

Introducing the leafScripts map in ScriptSortBuilder makes sense to me, but I have trouble reproducing the issue without your changes with the added tests. Left a comment around that for clarification.

cbuescher · 2025-03-06T12:24:33Z

...er/src/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/TopHitsIT.java

+                }
+            }
+        );
+    }


I'm trying to see this test fail without the changes to ScriptSortBuilder and the two field comparators but I could not get it to fail so far, even with several hundred of iterations and several re-runs with a different root seed. I see the index contains 50 docs, how are we sure its likely to run into using several segments / concurrent situations? I remember some tweaks to our integration tests to make this more likely for small indices but cannot remember, do you recall?

javanna · 2025-03-07T09:14:29Z

Looks like this fails with a legitimate and related failure:

thanks, I had seen the failure, it's a test issue to do with MockScriptEngine, and the eager call to get_score that I added when using top_hits and top_metrics.

Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause.

elasticsearchmachine · 2025-03-10T20:13:16Z

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	8.18
✅	8.x
✅	8.17

Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause.

* Fix concurrency issue in ScriptSortBuilder (#123757) Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause. * iter

Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause.

* Fix concurrency issue in ScriptSortBuilder (#123757) Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause. * iter

Inter-segment concurrency is disabled whenever sort by field, included script sorting, is used in a search request. The reason why sort by field does not use concurrency is that there are some performance implications, given that the hit queue in Lucene is build per slice and the different search threads don't share information about the documents they have already visited etc. The reason why script sort has concurrency disabled is that the script sorting implementation is not thread safe. This commit addresses such concurrency issue and re-enables search concurrency for search requests that use script sorting. In addition, missing tests are added to cover for sort scripts that rely on _score being available and top_hits aggregation with a scripted sort clause.

With elastic#123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with elastic#123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

With #123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with #123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

With elastic#123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with elastic#123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

…26012) With #123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with #123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

…26013) With #123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with #123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

…26011) With #123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with #123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

…26014) With #123610 we disabled parallel collection for field and script sorted top hits, aligning its behaviour with that of top level search. This was mainly to work around a bug in script sorting that did not support inter-segment concurrency. The bug with script sort has been fixed with #123757 and concurrency re-enabled for it. While sort by field is not optimized for search concurrency, top hits benefits from it and disabling concurrency for sort by field in top hits has caused performance regressions in our nightly benchmarks. This commit re-enables concurrency for top hits with sort by field is used. This introduces back a discrepancy between top level search and top hits, in that concurrency is applied for top hits despite sort by field normally disables it. The key difference is the context where sorting is applied, and the fact that concurrency is disabled only for performance reasons on top level searches and not for functional reasons.

javanna added 2 commits February 28, 2025 20:28

Fix concurrency issue in ScriptSortBuilder

7b37e1f

iter

19c68d8

elasticsearchmachine added the v9.1.0 label Feb 28, 2025

javanna and others added 6 commits March 1, 2025 10:10

wip

a9121f5

[CI] Auto commit changes from spotless

45a04c5

wip

406ce12

[CI] Auto commit changes from spotless

70965d5

Merge branch 'main' into fix/script_sort_concurrency

ae69a21

iter

68c3ac7

javanna added :Search Foundations/Search Catch all for Search Foundations v9.0.0 v8.18.1 v8.19.0 v8.17.4 auto-backport Automatically create backport pull requests when merged >bug labels Mar 5, 2025

javanna marked this pull request as ready for review March 5, 2025 23:02

javanna requested a review from a team as a code owner March 5, 2025 23:02

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Mar 5, 2025

Update docs/changelog/123757.yaml

621d998

javanna commented Mar 5, 2025

View reviewed changes

javanna requested a review from cbuescher March 5, 2025 23:06

[CI] Auto commit changes from spotless

620214d

cbuescher reviewed Mar 6, 2025

View reviewed changes

iter

1d6279b

Merge branch 'main' into fix/script_sort_concurrency

2971d5b

javanna merged commit def4c89 into elastic:main Mar 10, 2025
17 checks passed

javanna deleted the fix/script_sort_concurrency branch March 10, 2025 20:10

This was referenced Mar 10, 2025

[9.0] Fix concurrency issue in ScriptSortBuilder (#123757) #124513

Merged

[8.18] Fix concurrency issue in ScriptSortBuilder (#123757) #124514

Merged

This was referenced Mar 10, 2025

[8.x] Fix concurrency issue in ScriptSortBuilder (#123757) #124516

Merged

[8.17] Fix concurrency issue in ScriptSortBuilder (#123757) #124517

Merged

KunjueYu mentioned this pull request Mar 12, 2025

Simplify the logic of script sort parallel collection #124639

Open

javanna mentioned this pull request Mar 31, 2025

Re-enable parallel collection for field sorted top hits #125916

Merged

Fix concurrency issue in ScriptSortBuilder #123757

Fix concurrency issue in ScriptSortBuilder #123757

Uh oh!

Conversation

javanna commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 5, 2025

Uh oh!

elasticsearchmachine commented Mar 5, 2025

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!

original-brownbear commented Mar 6, 2025

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Mar 7, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 10, 2025

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

javanna commented Feb 28, 2025 •

edited

Loading