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

Add dh-defaults.vmoptions file #3057

Merged
merged 4 commits into from
Dec 1, 2022
Merged

Add dh-defaults.vmoptions file #3057

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56138fe
Add dh-defaults.vmoptions file
devinrsmith Nov 2, 2022
b49866c
Review
devinrsmith Nov 3, 2022
0b7d06d
Add options to py embedded server
devinrsmith Nov 4, 2022
72d9841
Add defaultJvmOptions.
devinrsmith Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions buildSrc/src/main/groovy/io.deephaven.java-toolchain-conventions.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,30 +123,54 @@ def devJvmArgs = [
// '-XX:+PrintCompilation', // this optional line shows jit operations as they happen
]

def createVmOptions = tasks.register('createVmOptions') {
def vmOptionsFile = project.layout.buildDirectory.file('dh-default.vmoptions')
it.outputs.file(vmOptionsFile)
doFirst {
// These are supposed to be generally applicable and recommended JVM options, but they aren't hard requirements.
// Overly specific options do *not* belong here. For example, we should _not_ be setting something like `-Xmx4g` here.
// If you are tempted to try and put system properties here (`-Dkey=value`), think again; there should be a more
// appropriate place to set those.
//
// From the perspective of our application distribution, these options will be used as defaults for JAVA_OPTS
// (if the user already has JAVA_OPTS set, these VM options will _not_ apply).
vmOptionsFile.get().asFile.text = "-XX:+UseG1GC\n-XX:MaxGCPauseMillis=100\n-XX:+UseStringDeduplication\n"
devinrsmith marked this conversation as resolved.
Show resolved Hide resolved
}
}

tasks.withType(JavaExec).configureEach {
def compilerDirectivesFile = createCompilerDirectives.get().outputs.files
def vmOptionsFile = createVmOptions.get().outputs.files
inputs.files compilerDirectivesFile
inputs.files vmOptionsFile
javaLauncher.set runtimeLauncher
jvmArgs += compilerArgs(compilerDirectivesFile.singleFile.path) + devJvmArgs
// Note: we _could_ have the vmOptionsFile constituents directly listed instead of using -XX:VMOptionsFile.
// That said, the current approach used here more closely matches how the application start script is defined.
jvmArgs += compilerArgs(compilerDirectivesFile.singleFile.path) + ["-XX:VMOptionsFile=${vmOptionsFile.singleFile.path}"] + devJvmArgs
}

tasks.withType(Test).configureEach {
def compilerDirectivesFile = createCompilerDirectives.get().outputs.files
def vmOptionsFile = createVmOptions.get().outputs.files
inputs.files compilerDirectivesFile

inputs.files vmOptionsFile
javaLauncher.set testRuntimeLauncher
jvmArgs += compilerArgs(compilerDirectivesFile.singleFile.path) + devJvmArgs
// Note: we _could_ have the vmOptionsFile constituents directly listed instead of using -XX:VMOptionsFile.
// That said, the current approach used here more closely matches how the application start script is defined.
jvmArgs += compilerArgs(compilerDirectivesFile.singleFile.path) + ["-XX:VMOptionsFile=${vmOptionsFile.singleFile.path}"] + devJvmArgs
}

tasks.withType(GroovyCompile).configureEach {
javaLauncher.set groovyCompilerLauncher
}

plugins.withType(ApplicationPlugin) {
applicationDistribution.into('lib') {
applicationDistribution.into('etc') {
rcaudy marked this conversation as resolved.
Show resolved Hide resolved
from(createCompilerDirectives.get().outputs.files)
from(createVmOptions.get().outputs.files)
}
}

tasks.withType(CreateStartScripts).configureEach {
def unixStartScript = resources.text.fromUri(getClass().classLoader.getResource('unixStartScript.txt'))
inputs.files unixStartScript
Expand Down
8 changes: 7 additions & 1 deletion buildSrc/src/main/resources/unixStartScript.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,13 @@ APP_BASE_NAME=`basename "\$0"`
DEFAULT_JVM_OPTS=${defaultJvmOpts}

# Customization for deephaven-core to reference a compiler directives file
DEFAULT_JVM_OPTS="\${DEFAULT_JVM_OPTS} -XX:+UnlockDiagnosticVMOptions -XX:CompilerDirectivesFile=\"\${APP_HOME}/lib/dh-compiler-directives.txt\""
# There is no easy way for users to override these options
DEFAULT_JVM_OPTS="\${DEFAULT_JVM_OPTS} -XX:+UnlockDiagnosticVMOptions -XX:CompilerDirectivesFile=\"\${APP_HOME}/etc/dh-compiler-directives.txt\""

# Customization for deephaven-core to include vm options file by default
# If users explicitly set JAVA_OPTS, they will be opting out of dh-default.vmoptions
DH_DEFAULT_VMOPTIONS="-XX:VMOptionsFile=\"\${APP_HOME}/etc/dh-default.vmoptions\""
JAVA_OPTS="\${JAVA_OPTS:-\${DH_DEFAULT_VMOPTIONS}}"

# Use the maximum available, or set MAX_FD != -1 to use that value.
MAX_FD="maximum"
Expand Down
3 changes: 1 addition & 2 deletions docker/server-jetty/src/main/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ HEALTHCHECK --interval=3s --retries=3 --timeout=11s CMD /bin/grpc_health_probe -
ENV \
DEEPHAVEN_CACHE_DIR="/cache" \
DEEPHAVEN_CONFIG_DIR="/opt/deephaven/config" \
DEEPHAVEN_DATA_DIR="/data" \
JAVA_OPTS="-XX:+UseG1GC -XX:MaxGCPauseMillis=100 -XX:+UseStringDeduplication"
DEEPHAVEN_DATA_DIR="/data"
ENTRYPOINT [ "/opt/deephaven/server/bin/start" ]
ARG DEEPHAVEN_VERSION
ARG SERVER
Expand Down
2 changes: 1 addition & 1 deletion docker/server-slim/src/main/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ ENV \
DEEPHAVEN_CACHE_DIR="/cache" \
DEEPHAVEN_CONFIG_DIR="/opt/deephaven/config" \
DEEPHAVEN_DATA_DIR="/data" \
JAVA_OPTS="-XX:+UseG1GC -XX:MaxGCPauseMillis=100 -XX:+UseStringDeduplication -Ddeephaven.console.type=groovy"
START_OPTS="-Ddeephaven.console.type=groovy"
ENTRYPOINT [ "/opt/deephaven/server/bin/start" ]

ARG DEEPHAVEN_VERSION
Expand Down
3 changes: 1 addition & 2 deletions docker/server/src/main/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ HEALTHCHECK --interval=3s --retries=3 --timeout=11s CMD /bin/grpc_health_probe -
ENV \
DEEPHAVEN_CACHE_DIR="/cache" \
DEEPHAVEN_CONFIG_DIR="/opt/deephaven/config" \
DEEPHAVEN_DATA_DIR="/data" \
JAVA_OPTS="-XX:+UseG1GC -XX:MaxGCPauseMillis=100 -XX:+UseStringDeduplication"
DEEPHAVEN_DATA_DIR="/data"
ENTRYPOINT [ "/opt/deephaven/server/bin/start" ]
ARG DEEPHAVEN_VERSION
ARG SERVER
Expand Down
4 changes: 4 additions & 0 deletions py/embedded-server/deephaven_server/start_jvm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ def _jars_path():
def _compiler_directives():
return _jars_path() / 'dh-compiler-directives.txt'

def _default_vmoptions():
return _jars_path() / 'dh-default.vmoptions'

def _jars():
return _jars_path().glob('*.jar')

Expand All @@ -42,6 +45,7 @@ def _jars():
f"-XX:CompilerDirectivesFile={_compiler_directives()}",
# (deephaven-core#2500): Remove DisableIntrinsic for currentThread
'-XX:DisableIntrinsic=_currentThread',
f"-XX:VMOptionsFile={_default_vmoptions()}",
]

# Provide a util func to start the JVM, will use its own defaults if none are offered
Expand Down
1 change: 1 addition & 0 deletions py/embedded-server/java-runtime/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def serverClasspath = tasks.register('serverClasspath', Sync) {
from configurations.runtimeClasspath
from jar
from tasks.named('createCompilerDirectives')
from tasks.named('createVmOptions')
into layout.buildDirectory.dir('classpath')
}

Expand Down
4 changes: 0 additions & 4 deletions server/jetty-app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ if (hasProperty('quiet')) {
tasks.withType(JavaExec).configureEach {
// This appends to the existing jvm args, so that java-open-nio still takes effect
jvmArgs extraJvmArgs

jvmArgs '-XX:+UseG1GC',
'-XX:MaxGCPauseMillis=100',
'-XX:+UseStringDeduplication'
}

tasks.withType(CreateStartScripts).configureEach {
Expand Down
6 changes: 0 additions & 6 deletions server/netty-app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ if (hasProperty('quiet')) {
tasks.withType(JavaExec).configureEach {
// This appends to the existing jvm args, so that java-open-nio still takes effect
jvmArgs extraJvmArgs

jvmArgs '-XX:+UseG1GC',
'-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.

}

tasks.withType(CreateStartScripts).configureEach {
Expand Down