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 dh-defaults.vmoptions file #3057

Merged
merged 4 commits into from
Dec 1, 2022

Conversation

devinrsmith
Copy link
Member

Consolidates the recommended VM options into a single place. Allows for these options to be defined in a single place.

Moves dh-compiler-directives.txt and dh-defaults.vmoptions into the etc directory (instead of in lib alongside the jars).

Fixes #3054

Consolidates the recommended VM options into a single place. Allows for these options to be defined in a single place.

Moves dh-compiler-directives.txt and dh-defaults.vmoptions into the etc directory (instead of in lib alongside the jars).

Fixes deephaven#3054
buildSrc/src/main/resources/unixStartScript.txt Outdated Show resolved Hide resolved
'-XX:MaxGCPauseMillis=100',
'-XX:+UseStringDeduplication',
// For development in this repository via Gradle, use the top-level data directory
"-Dstorage.path=${rootDir}/data"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need/want this anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The defaults should be good enough IMO.

The equivalent was removed from jetty-app, and was accidentally left behind here. storage.path defaults to <deephaven.dataDir>/storage, and for local development (prepareCompose, docker compose up) that equates to ${rootDir}/data/storage given:

    volumes:
      - ./data:/data

For native local development (./gradlew ... :run), the default will use OS conventional paths.

Copy link
Member

Choose a reason for hiding this comment

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

Might changing this break persisted notebooks for users? Do we need to put out a notice of the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

This image is used for local development only.

@rcaudy
Copy link
Member

rcaudy commented Nov 3, 2022

This needs testing on each of our supported JVM major versions, I presume?

@devinrsmith
Copy link
Member Author

It's officially documented as available on 11 and 17, but I'll make sure to do some explicit tests on a variety of JVMs - will report back.

@devinrsmith
Copy link
Member Author

I've successfully tested on Temurin 11, 17, and 19 as well as Graal 22.3.0 11, 17, and 19 on Linux.

@devinrsmith devinrsmith requested a review from niloc132 November 3, 2022 02:30
rcaudy
rcaudy previously approved these changes Nov 3, 2022
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

Approved. Do note: #3057 (comment)

Be a bit pedantic with setting inputs property which fixes a theoretical caching issue:

```java
def defaultJvmOptions = [
  '-XX:+UseG1GC',
  '-XX:MaxGCPauseMillis=100',
  '-XX:+UseStringDeduplication',
  "-Dbuild.time=${System.currentTimeMillis()}"
]
```

Without inputs.property, the task would be improperly cached.
@devinrsmith devinrsmith requested a review from niloc132 November 16, 2022 03:05
@devinrsmith devinrsmith added the ReleaseNotesNeeded Release notes are needed label Dec 1, 2022
@devinrsmith devinrsmith requested a review from rcaudy December 1, 2022 19:07
@devinrsmith devinrsmith merged commit 106bdf8 into deephaven:main Dec 1, 2022
@devinrsmith devinrsmith deleted the add-vm-opts-file branch December 1, 2022 21:51
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2022
@deephaven-internal
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce duplication for suggested defaults for java VM options
4 participants