Skip to content
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

Add JIT compiler excludes for computeCommonPrefixLengthAndBuildHistogram #103112

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

ChrisHegarty
Copy link
Contributor

@ChrisHegarty ChrisHegarty commented Dec 7, 2023

This commit adds a temporary JIT compile command that excludes the compilation of a couple of Lucene methods which, if compiled, crash the JVM.

The excludes are added to:

  1. The Gradle test plugin, so that tests run with the Gradle runner will not compile these methods - this the most impactful change as we see RangeAggregatorTests failing very frequently when run with gradle :server:test.

  2. The distribution jvm.options, so that the distribution is protected from the potential crash. A consequence of this is that the log output will now emit a notice of these excludes - this is benign, e.g.

    $ ./build/distribution/local/elasticsearch-8.12.0-SNAPSHOT/bin/elasticsearch
    CompileCommand: exclude org/apache/lucene/util/MSBRadixSorter.computeCommonPrefixLengthAndBuildHistogram bool exclude = true
    CompileCommand: exclude org/apache/lucene/util/RadixSelector.computeCommonPrefixLengthAndBuildHistogram bool exclude = true
    Dec 07, 2023 10:35:02 AM sun.util.locale.provider.LocaleProviderAdapter <clinit>
    WARNING: COMPAT locale provider will be removed in a future release
    ...
    

One consequence of no.1 is that RangeAggregatorTests may still crash when run directly in the IDE, but this is of a lesser concern. And since the workaround is temporary (until JDK 21.0.2 is released), then this should be ok.

More specifics can be found in #103004, which I will not repeat here.

@ChrisHegarty ChrisHegarty added >bug :Search/Search Search-related issues that do not fall into other categories jvm bug Team:Core/Infra Meta label for core/infra team Team:Search Meta label for search team Team:Delivery Meta label for Delivery team labels Dec 7, 2023
@elasticsearchmachine elasticsearchmachine added v8.13.0 and removed Team:Core/Infra Meta label for core/infra team Team:Delivery Meta label for Delivery team labels Dec 7, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@javanna
Copy link
Member

javanna commented Dec 7, 2023

It's great that we have a fix for Elasticsearch and get unblocked quickly, yet it would be good to protect Lucene from this, given that other Lucene users are exposed to it until the fix is released with the jdk?

@ChrisHegarty
Copy link
Contributor Author

It's great that we have a fix for Elasticsearch and get unblocked quickly, yet it would be good to protect Lucene from this, given that other Lucene users are exposed to it until the fix is released with the jdk?

Indeed. This PR is intended as a temporary measure so that we can avoid the backout of the Lucene 9.9.0 upgrade in ES, and restore stability to ES.

I am continuing to investigate a possible workaround in Lucene code, which if possible would require a new Lucene release, Maybe 9.9.1 ? (but that not something that we can decide here)

@ChrisHegarty
Copy link
Contributor Author

ChrisHegarty commented Dec 7, 2023

@elasticmachine retest this please

Clean CI run, but lets do it again.

2 similar comments
@ChrisHegarty
Copy link
Contributor Author

@elasticmachine retest this please

Clean CI run, but lets do it again.

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine retest this please

Clean CI run, but lets do it again.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrisHegarty ChrisHegarty merged commit b510e59 into elastic:main Dec 7, 2023
14 of 15 checks passed
ChrisHegarty added a commit to ChrisHegarty/elasticsearch that referenced this pull request Dec 7, 2023
…ram (elastic#103112)

This commit adds a temporary JIT compile command that excludes the compilation of a couple of Lucene methods which, if compiled, crash the JVM.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.12

@@ -108,7 +108,10 @@ public void execute(Task t) {
"--add-opens=java.base/java.nio.file=ALL-UNNAMED",
"--add-opens=java.base/java.time=ALL-UNNAMED",
"--add-opens=java.management/java.lang.management=ALL-UNNAMED",
"-XX:+HeapDumpOnOutOfMemoryError"
"-XX:+HeapDumpOnOutOfMemoryError",
// REMOVE once bumped to a JDK greater than 21.0.1, https://github.com/elastic/elasticsearch/issues/103004
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we'll be able to remove this just because of a JDK upgrade since we still test on older JDKs, unless this fix is backported to JDK 17.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair comment @mark-vieira. We're currently in the process of investigating a possible "fix" in Lucene. If this were to happen then these options can be removed. If not, then I'll amend the comment to something more appropriate for its longer term presence.

@@ -0,0 +1,5 @@
pr: 103112
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I'm not sure we need to include a changelog entry given this "bug" never existed in a released version.

elasticsearchmachine pushed a commit that referenced this pull request Dec 8, 2023
…ram (#103112) (#103142)

This commit adds a temporary JIT compile command that excludes the compilation of a couple of Lucene methods which, if compiled, crash the JVM.

Co-authored-by: Elastic Machine <[email protected]>
ChrisHegarty added a commit that referenced this pull request Dec 19, 2023
This commit upgrades to Lucene 9.9.1.

With the upgrade to 9.9.1 we can now remove the compiler excludes added by #103112.
navarone-feekery pushed a commit to navarone-feekery/elasticsearch that referenced this pull request Dec 22, 2023
This commit upgrades to Lucene 9.9.1.

With the upgrade to 9.9.1 we can now remove the compiler excludes added by elastic#103112.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug jvm bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.12.0 v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants