Re-enable single parsing of compiler graphs in native-image #19511
Conversation
GraalVM 21.2-dev at some point introduced a new enhancement (enabled by default) to avoid parsing compiler graphs twice. This enhancement however results in increased memory usage at the moment (oracle/graal#3435) which then results in Quarkus' Integration Test failures in CI. In GraalVM 21.2 this appears to no longer be an issue.
zakkak
force-pushed
the
enable-parseonce
branch
from
b0b2198 to
054236b
Compare
|
Testing this with a more recent Quarkus seems OK except for the following error in Passing @Sanne it looks like when ParseOnce and InlineBeforeAnalysis are both enabled (default behavior in the upcoming GraalVM 21.3) doesn't work as expected. My first thought was that diff --git a/extensions/jdbc/jdbc-postgresql/runtime/src/main/java/io/quarkus/jdbc/postgresql/runtime/graal/PgSQLXML.java b/extensions/jdbc/jdbc-postgresql/runtime/src/main/java/io/quarkus/jdbc/postgresql/runtime/graal/PgSQLXML.java
index b0333866f2..2dea4eaeda 100644
--- a/extensions/jdbc/jdbc-postgresql/runtime/src/main/java/io/quarkus/jdbc/postgresql/runtime/graal/PgSQLXML.java
+++ b/extensions/jdbc/jdbc-postgresql/runtime/src/main/java/io/quarkus/jdbc/postgresql/runtime/graal/PgSQLXML.java
@@ -41,6 +41,7 @@ import org.postgresql.xml.PGXmlFactoryFactory;
import org.xml.sax.InputSource;
import org.xml.sax.XMLReader;
+import com.oracle.svm.core.annotate.NeverInline;
import com.oracle.svm.core.annotate.Substitute;
import com.oracle.svm.core.annotate.TargetClass;
@@ -222,6 +223,7 @@ public final class PgSQLXML implements SQLXML {
return stringWriter;
}
+ @NeverInline("Prevent inlining so that io.quarkus.jdbc.postgresql.runtime.graal.SQLXLMFeature keeps working with -H:+InlineBeforeAnalysis")
@Substitute
@Override
public synchronized <T extends Result> T setResult(Class<T> resultClass) throws SQLException {But the test still fails. @Sanne any thoughts? (+ FYI) |
|
After some profiling it looks like the use of ParseOnce is no longer causing issues with the CI (like the ones mentioned in #17711) both in 21.2 and 21.3-dev (averages of 3 runs) 21.3-dev
21.2
Especially motivated by the reduction of the allocated heap size in 21.3-dev I believe we should allow GraalVM/Mandrel to keep ParseOnce enabled moving forward. Note: Since ParseOnce in conjuntion with InlineBeforeAnalysis is causing issues with one test (see #19511 (comment)) I opted for keeping InlineBeforeAnalysis disabled even when using 21.3 till we figure it out. According to my measurements InlineBeforeAnalysis has no significant impact on compilation times or memory utilization. However, it's supposed to enable GraalVM to take advantage of inline methods to improve the precision of the static analysis (see oracle/graal#3818) and thus it's expected to produce more efficient native executables. That said, we should try to enable it at some point. |
|
I can't say I know much about the subject. so I'll leave this to others. |
|
Hi @zakkak , if I was able to identify the But having changed the analysis algorythms, I suppose other tricky paths might have materialized? It's very likely that it's being included via a different path now, as my patches on I think it's really important to keep such types out of it as they come with a significant amount of dependencies; however it might be more interesting for the long term to figure out why GraalVM isn't able to see such paths as dead code automatically. |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 5a52322
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖ ⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖ |
zakkak
force-pushed
the
enable-parseonce
branch
from
5a52322 to
c8187f5
Compare
|
@Sanne it turns out what is happening with ParseOnce + InlineBeforeAnalysis is that GraalVM is now able to figure out the trick in so it automatically registers it for reflection. Fixed in c8187f5 Please re-review :) new CI runs for Mandrel 21.2: https://github.com/graalvm/mandrel/actions/runs/1282221217 |
|
ah awesome :) Sorry I should have remembered of that one. |
Starting with GraalVM 21.3 InlineBeforeAnalysis is going to be enabled by default. This patch enables it for 21.2 as well.
zakkak
force-pushed
the
enable-parseonce
branch
from
c8187f5 to
b9e22a5
Compare
|
Updated PR following @Sanne's comment c8187f5#commitcomment-57177705 New CI runs for Mandrel 21.2: https://github.com/graalvm/mandrel/actions/runs/1283152824 |
Failing Jobs - Building b9e22a5
Full information is available in the Build summary check run. Failures⚙️ MicroProfile TCKs Tests #- Failing: tcks/microprofile-fault-tolerance
📦 tcks/microprofile-fault-tolerance✖ |
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main.
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main.
|
Removing Note: This is true for both 21.2 and 21.3 |
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main.
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main.
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main. (cherry picked from commit 201b9a6)
Disable single parsing of compiler graphs till the impact of it on heap usage decreases see oracle/graal#3435 and graalvm/mandrel#304 (comment) quarkusio#19511 didn't take into account runs with constraint memory. Bringing back `-H:-ParseOnce` reduces the heap usage significantly, from 3.9G down to 2.7G in integration-tests/main. (cherry picked from commit 201b9a6)
GraalVM 21.2-dev introduced a new enhancement (enabled by default) to avoid parsing compiler graphs twice.
This enhancement however resulted in increased memory usage (oracle/graal#3435) which then resulted in Quarkus' Integration Test failures in CI.
In GraalVM 21.2 and 21.3-dev this appears to no longer be an issue.
I have tested this branch with GraalVM CE and Mandrel 21.2, as well as with GraalVM CE and Mandrel 21.3-dev.
Things look good except for
io.quarkus.it.jpa.postgresql.JPAFunctionalityInGraalITCasewhich fails when compiled with 21.3-dev with:Keeping this as draft for now.Closes: #19446