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

Disable preventive GC #15380

Merged
merged 1 commit into from Dec 15, 2022
Merged

Disable preventive GC #15380

merged 1 commit into from Dec 15, 2022

Conversation

Chaho12
Copy link
Member

@Chaho12 Chaho12 commented Dec 13, 2022 •
edited

Description

Preventive GC was added in 17, but has issues so JDK 20 disables it by default (plans to completely remove it in JDK 21)
Users should be aware that until Trino uses java 20+, they should not use preventive GC as it could lead to unexpected OOM

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 13, 2022
@Chaho12 Chaho12 requested a review from mosabua December 13, 2022 04:46
@github-actions github-actions bot added the docs label Dec 13, 2022
@wendigo
Copy link
Contributor

wendigo commented Dec 13, 2022

I think that we should add this to the default jvm.config files.

cc @trinodb/maintainers

@findepi
Copy link
Member

findepi commented Dec 13, 2022

Preventive GC was added in 17

Is it enabled by default?

@Chaho12
Copy link
Member Author

Chaho12 commented Dec 13, 2022

Is it enabled by default?

Yes, so a pr was created to disable it by default. However, it is only applied to java 20 and not 17.

@findepi findepi changed the title Document to disable preventive GC Disable preventive GC Dec 13, 2022
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/add-preventivegc-docs branch from 02665cb to c0fcec9 Compare December 14, 2022 01:23
@findepi
Copy link
Member

findepi commented Dec 14, 2022

Please update the RPM config and product tests too.

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/add-preventivegc-docs branch from c0fcec9 to 1a1c034 Compare December 14, 2022 08:22
@Chaho12
Copy link
Member Author

Chaho12 commented Dec 14, 2022

Please update the RPM config and product tests too.

I updated these jvm.configs files, is this what you meant by rpm config and product tests? I searched all usages for -XX:+UnlockDiagnosticVMOptions and added -XX:-G1UsePreventiveGC option.
image

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/add-preventivegc-docs branch from 1a1c034 to 985c135 Compare December 14, 2022 09:22
losipiuk
losipiuk reviewed Dec 14, 2022
View reviewed changes
docs/src/main/sphinx/installation/deployment.rst Outdated
@@ -156,6 +158,7 @@ temporary directory by adding ``-Djava.io.tmpdir=/path/to/other/tmpdir`` to the
list of JVM options.

We enable ``-XX:+UnlockDiagnosticVMOptions`` and ``-XX:+UseAESCTRIntrinsics`` to improve AES performance for S3, etc. on ARM64 (`JDK-8271567 <https://bugs.openjdk.java.net/browse/JDK-8271567>`_)
We disable prevent GC (``-XX:-G1UsePreventiveGC``) for performance reasons (see `JDK-8293861 <https://bugs.openjdk.org/browse/JDK-8293861>`_)
Copy link
Member

Choose a reason for hiding this comment

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

nit: prevent -> preventive

Copy link
Member

Choose a reason for hiding this comment

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

Also change

# Prevent GC for performance reasons (JDK-8293861)

to

# Disable Preventive GC for performance reasons (JDK-8293861)

everywhere

Copy link
Member

Choose a reason for hiding this comment

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

thanks @losipiuk for correcting my mistakes (#15380 (comment))

losipiuk
losipiuk approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM % nits

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/add-preventivegc-docs branch from 985c135 to d57e2d8 Compare December 14, 2022 15:30
@Chaho12
Copy link
Member Author

Chaho12 commented Dec 15, 2022

I don't quite understand why it failed for presto v350 version :(
Also, I removed preventive GC for parallel GC test (preventive GC only exists for G1GC)

image

@wendigo
Copy link
Contributor

wendigo commented Dec 15, 2022

@Chaho12 v350 is running on JDK11

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/add-preventivegc-docs branch from f6f94e2 to ef53984 Compare December 15, 2022 06:22
@Chaho12
Copy link
Member Author

Chaho12 commented Dec 15, 2022

@Chaho12 v350 is running on JDK11

Oh i see. I removed -XX:-G1UsePreventiveGC from https://github.com/trinodb/trino/blob/master/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/presto/etc/jvm.config

@losipiuk losipiuk merged commit 4a65f20 into trinodb:master Dec 15, 2022
@wendigo
Copy link
Contributor

wendigo commented Dec 15, 2022

Thank you, @Chaho12, for contributing this!

@github-actions github-actions bot added this to the 404 milestone Dec 15, 2022
@Chaho12 Chaho12 deleted the feature/jaeho.yoo/add-preventivegc-docs branch December 19, 2022 09:09
@colebow colebow mentioned this pull request Dec 21, 2022
Merged
@arhimondr arhimondr mentioned this pull request Jan 6, 2023
Open
@caojoshua caojoshua mentioned this pull request Sep 28, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

@losipiuk losipiuk losipiuk approved these changes

@mosabua mosabua Awaiting requested review from mosabua

Assignees
No one assigned
Labels
cla-signed docs
Milestone
405
Development

Successfully merging this pull request may close these issues.

Disable preventive garbage collection for java 17
5 participants
@Chaho12 @wendigo @findepi @mosabua @losipiuk