Skip to content

Revert "Enable madvise by default for all builds (#110159)" #126308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2025

Conversation

ChrisHegarty
Copy link
Contributor

This reverts commit 4a77e06. We've seen a significant performance degradation in merging vectors resulting from the use of MADV_RANDOM and MGLRU ( and LRU in recent Linux kernels )

For the 8.x release train, then we will revert the change that enabled MADV_RANDOM. And backport to all shipping 8.x bugfix releases.

For 9.x the perf regression is less significant, and we will continue to investigate a more complete solution within Lucene, see, apache/lucene#14408.

relates: #124499

@ChrisHegarty ChrisHegarty added Team:Performance Meta label for performance team :Search Relevance/Vectors Vector search Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Apr 4, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@elasticsearchmachine elasticsearchmachine added v8.19.0 and removed Team:Performance Meta label for performance team labels Apr 4, 2025
@@ -36,6 +37,8 @@

public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {

private static final FeatureFlag MADV_RANDOM_FEATURE_FLAG = new FeatureFlag("madv_random");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok to keep the feature flag, since it could be enabled in non-vector scenarios. But I'm also ok to either just remove it or replace it with a system property, if we wanna retain the ability to enable this but not even have it on-by-default in snapshot builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisHegarty if we wish to tranform it into a regular system property, we can do that in a separate PR. However, I am not sure we want to expose it at all in v8.x as its incredibly trappy.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should label this as a "bug" to ensure that its called out in release notes.

@@ -36,6 +37,8 @@

public class FsDirectoryFactory implements IndexStorePlugin.DirectoryFactory {

private static final FeatureFlag MADV_RANDOM_FEATURE_FLAG = new FeatureFlag("madv_random");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisHegarty if we wish to tranform it into a regular system property, we can do that in a separate PR. However, I am not sure we want to expose it at all in v8.x as its incredibly trappy.

@benwtrent benwtrent added the >bug label Apr 4, 2025
@ChrisHegarty ChrisHegarty merged commit dabea23 into elastic:8.x Apr 4, 2025
14 of 15 checks passed
ChrisHegarty added a commit that referenced this pull request Apr 4, 2025
This reverts commit 4a77e06. We've seen a significant performance degradation in merging vectors resulting from the use of MADV_RANDOM and MGLRU ( and LRU in recent Linux kernels )

For the 8.x release train, then we will revert the change that enabled MADV_RANDOM. And backport to all shipping 8.x bugfix releases.

relates: #124499
ChrisHegarty added a commit that referenced this pull request Apr 4, 2025
This reverts commit 4a77e06. We've seen a significant performance degradation in merging vectors resulting from the use of MADV_RANDOM and MGLRU ( and LRU in recent Linux kernels )

For the 8.x release train, then we will revert the change that enabled MADV_RANDOM. And backport to all shipping 8.x bugfix releases.

relates: #124499
@ChrisHegarty ChrisHegarty deleted the random_revert branch April 4, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Relevance/Vectors Vector search Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.17.5 v8.18.1 v8.19.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants