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

Optimize Accepts header validation in the common case #15585

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 9, 2021

The common case of having multiple Accepts headers validate
against a single @produces media type shows up often
(as both browsers and benchmarks send 5 values in the Accepts
header)

The common case of having multiple Accepts headers validate
against a single @produces media type shows up often
(as both browsers and benchmarks send 5 values in the Accepts
header)
@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

In my simple hello world style tests with

application/json,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7

as the value of the Accepts header, this have an improvement of from 1 to 2 percent.

@geoand geoand requested a review from stuartwdouglas March 9, 2021 16:26
@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

@gsmet you might want to know about this:

 Error:  Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
Error: r]: Build step io.quarkus.deployment.pkg.steps.NativeImageBuildStep#build threw an exception: java.lang.RuntimeException: Failed to build native image
Error:  	at io.quarkus.deployment.pkg.steps.NativeImageBuildStep.build(NativeImageBuildStep.java:209)
Error:  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error:  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
Error:  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Error:  	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
Error:  	at io.quarkus.deployment.ExtensionLoader$2.execute(ExtensionLoader.java:920)
Error:  	at io.quarkus.builder.BuildContext.run(BuildContext.java:277)
Error:  	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2415)
Error:  	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1452)
Error:  	at java.base/java.lang.Thread.run(Thread.java:834)
Error:  	at org.jboss.threads.JBossThread.run(JBossThread.java:501)
Error:  Caused by: java.nio.file.NoSuchFileException: D:\a\quarkus\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-native-image-source-jar\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.exe

@gsmet
Copy link
Member

gsmet commented Mar 9, 2021

Nice:

Exception during JVMCI compiler initialization
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (./src/hotspot/share/jvmci/jvmciRuntime.cpp:1120), pid=4756, tid=68
#  fatal error: Fatal exception in JVMCI: Exception during JVMCI compiler initialization
#
# JRE version: OpenJDK Runtime Environment GraalVM CE 21.0.0 (11.0.10+8) (build 11.0.10+8-jvmci-21.0-b06)
# Java VM: OpenJDK 64-Bit Server VM GraalVM CE 21.0.0 (11.0.10+8-jvmci-21.0-b06, mixed mode, tiered, jvmci, jvmci compiler, compressed oops, parallel gc, windows-amd64)
# Core dump will be written. Default location: D:\a\quarkus\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-native-image-source-jar\hs_err_pid4756.mdmp

hopefully, it won't happen very often :)

@geoand
Copy link
Contributor Author

geoand commented Mar 9, 2021

🤞🏼

@gsmet
Copy link
Member

gsmet commented Mar 9, 2021

BTW, I'm unhappy with the error management of this thing. It should report the build error of the native image process not the fact that the file does not exist. I wonder if it's a regression because I have a vague recollection it used to report the build error.

@zakkak @jonathan-meier could this behavior be related to your recent changes?

@jonathan-meier
Copy link
Contributor

jonathan-meier commented Mar 9, 2021

@gsmet I looked into it and it is unlikely to be a regression from our recent changes, but rather a pre-existing issue, though only on Windows (I guess that's why no one noticed until now). On Linux we should correctly report the build error.

The exception is thrown on line 185 (and then caught and rethrown on lines 208-209):

int exitCode = buildRunner.build(nativeImageArgs, outputDir, processInheritIODisabled.isPresent());
if (exitCode != 0) {
throw imageGenerationFailed(exitCode, nativeImageArgs);
}
Path generatedExecutablePath = outputDir.resolve(resultingExecutableName);
Path finalExecutablePath = outputTargetBuildItem.getOutputDirectory().resolve(resultingExecutableName);
IoUtils.copy(generatedExecutablePath, finalExecutablePath);
Files.delete(generatedExecutablePath);
if (nativeConfig.debug.enabled) {
final String sources = "sources";
final Path generatedSources = outputDir.resolve(sources);
final Path finalSources = outputTargetBuildItem.getOutputDirectory().resolve(sources);
IoUtils.copy(generatedSources, finalSources);
IoUtils.recursiveDelete(generatedSources);
}
System.setProperty("native.image.path", finalExecutablePath.toAbsolutePath().toString());
if (objcopyExists()) {
if (nativeConfig.debug.enabled) {
splitDebugSymbols(finalExecutablePath);
}
// Strip debug symbols regardless, because the underlying JDK might contain them
objcopy("--strip-debug", finalExecutablePath.toString());
} else {
log.warn("objcopy executable not found in PATH. Debug symbols will not be separated from executable.");
log.warn("That will result in a larger native image with debug symbols embedded in it.");
}
return new NativeImageBuildItem(finalExecutablePath);
} catch (Exception e) {
throw new RuntimeException("Failed to build native image", e);
} finally {
if (nativeConfig.debug.enabled) {
removeJarSourcesFromLib(outputTargetBuildItem);
IoUtils.recursiveDelete(outputDir.resolve(Paths.get(APP_SOURCES)));
}
}

This means we successfully passed the exit code check on line 180, meaning we received exit code 0 even though clearly native-compilation returned a non-zero exit code (1 in this particular case).

It turns out that since native-image.cmd does not explicitly exit with a proper exit code, we'll always receive 0 as an exit code from invoking native-image.cmd no matter what the actual invocation of native-image.exe inside native-image.cmd returned.

For reference, this is the content of native-image.cmd:

@echo off

call :getScriptLocation location
"%location%..\lib\svm\bin\native-image.exe" %*
:goto eof

:: If this script is in `%PATH%` and called quoted without a full path (e.g., `"js"`), `%~dp0` is expanded to `cwd`
:: rather than the path to the script.
:: This does not happen if `%~dp0` is accessed in a subroutine.
:getScriptLocation variableName
    set "%~1=%~dp0"
    exit /b 0

replacing :goto eof with exit %errorlevel% would solve the issue. Other than that, I don't know what other options we'd have in order to catch the exit code of native-image.exe except invoking it directly instead of going through the wrapper script.

@zakkak
Copy link
Contributor

zakkak commented Mar 10, 2021

replacing :goto eof with exit %errorlevel% would solve the issue.

FYI: This is fixed by oracle/graal@5cbc36f in GraalVM master.

@gsmet
Copy link
Member

gsmet commented Mar 10, 2021

Perfect, thanks guys!

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Makes perfect sense. We have tests covering that?

@geoand
Copy link
Contributor Author

geoand commented Mar 10, 2021

Yes, the TCK covers this stuff

@gsmet gsmet merged commit bade6cd into quarkusio:master Mar 10, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants