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.

Sign up for GitHub

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

Jump to bottom

Don't re-initialize ThreadLocalRandom in GraalVM > 20.2 #12344

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Don't re-initialize ThreadLocalRandom in GraalVM > 20.2 #12344

merged 1 commit into from
Sep 30, 2020

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Sep 25, 2020 •
edited
Loading

As of oracle/graal@1b7b2c8 reinitialization of ThreadLocalRandom causes failures in native image
builds (see oracle/graal#2841).
Additionally as of oracle/graal#2790 Quarkus doesn't need to manually reinitialize ThreadLocalRandom.

@zakkak zakkak force-pushed the fix-oracle-graal-2841 branch from 234718b to 6bda026 Compare September 25, 2020 13:33
@zakkak zakkak changed the title Don't re-initialize ThreadLocalRandom in GraalVM <= 20.2 Don't re-initialize ThreadLocalRandom in GraalVM > 20.2 Sep 25, 2020
@zakkak zakkak force-pushed the fix-oracle-graal-2841 branch from 6bda026 to d98e117 Compare September 25, 2020 13:34
@zakkak
Don't re-initialize ThreadLocalRandom in GraalVM > 20.2
51fc58b 
As of
oracle/graal@1b7b2c8
reinitialization of ThreadLocalRandom causes failures in native image
builds (see oracle/graal#2841). Additionally
as of oracle/graal#2790 Quarkus doesn't need to
manually reinitialize ThreadLocalRandom.
@zakkak zakkak force-pushed the fix-oracle-graal-2841 branch from d98e117 to 51fc58b Compare September 25, 2020 13:40
gastaldi
gastaldi approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@gsmet
Copy link
Member

gsmet commented Sep 28, 2020

Please refrain from having behavior changes and refactorings in the same commit. It makes things very hard to review.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 28, 2020

Please refrain from having behavior changes and refactorings in the same commit. It makes things very hard to review.

Noted. For the record the behavior changes in this PR are https://github.com/quarkusio/quarkus/pull/12344/files#diff-99b0ae6b0ce4104e45b9c9d5f6d3b8a6L669-R668 anything else is refactoring to reduce code duplication.

@gsmet gsmet merged commit 51a6f7d into quarkusio:master Sep 30, 2020
@gsmet gsmet added this to the 1.9.0 - master milestone Sep 30, 2020
@gsmet
Copy link
Member

gsmet commented Sep 30, 2020

@zakkak I understand that this should be backported as it's affecting current stable versions with GraalVM 20.2. Am I right? Thanks.

@zakkak
Copy link
Contributor Author

zakkak commented Sep 30, 2020

@zakkak I understand that this should be backported as it's affecting current stable versions with GraalVM 20.2. Am I right? Thanks.

Well one could indeed skip the explicit re-initialization with 20.2, but would need to enable the RandomNumbersFeature to achieve this (see oracle/graal#2790) and they would still miss oracle/graal@1b7b2c8 so I am not sure it would work as expected.

This PR only skips re-initialization for GraalVM > 20.2 since 20.3 will feature RandomNumbersFeature as an AutomaticFeature (see oracle/graal#2790) and will include both oracle/graal@1b7b2c8 (which fixes the initialization of ThreadLocalRandom but is also the one causing the Quarkus failures that this PR fixes) and oracle/graal@665fa48 (which fixes the native-image failure when explicitly reinitializing ThreadLocalRandom, making this PR an optimization instead of a bug fix).

HTH

@zakkak zakkak deleted the fix-oracle-graal-2841 branch September 30, 2020 15:52
@gsmet
Copy link
Member

gsmet commented Sep 30, 2020

OK, let's not backport it then. Thanks.

@zakkak zakkak mentioned this pull request Oct 1, 2020
Closed
This was referenced Dec 15, 2020
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.9.0.CR1
Development

Successfully merging this pull request may close these issues.
3 participants
@zakkak @gsmet @gastaldi