Bootstrap new jvm for running formatter with flags #562
Conversation
...bootstrap/src/main/java/com/palantir/javaformat/bootstrap/BootstrappingFormatterService.java
Outdated
Show resolved
Hide resolved
.../src/test/java/com/palantir/java/javaformat/bootstrap/BootstrappingFormatterServiceTest.java
Outdated
Show resolved
Hide resolved
fawind
changed the title
| @@ -37,7 +37,7 @@ private PalantirJavaFormatStep() {} | |||
| /** Creates a step which formats everything - code, import order, and unused imports. */ | |||
| public static FormatterStep create(Configuration palantirJavaFormat, JavaFormatExtension extension) { | |||
| ensureImplementationNotDirectlyLoadable(); | |||
| Supplier<FormatterService> memoizedService = extension::serviceLoad; | |||
| Supplier<FormatterService> memoizedService = extension::loadFormatterService; | |||
| return FormatterStep.createLazy( | |||
| NAME, () -> new State(palantirJavaFormat.getFiles(), memoizedService), State::createFormat); | |||
There was a problem hiding this comment.
The gradle integration is very slow because spotless invokes one formatter command per file which will now start a new jvm each. On a medium-sized project (205 java file), formatting all files took ~2 mins. Fortunately files are cached and not reformatted if they don't change.
One solution would be to keep a JVM hot and use this for all formatter invocations. However I would do this as a follow up PR to keep this one smaller. For now, the slow bootstrapping formatter is only used when using Java 16+ (can also make it opt-in to start with).
There was a problem hiding this comment.
We are running into similar problems with spotless in gradle-baseline (see diffplug/spotless#834). As this plugin is more integrated with Gradle, its not super easy to bootstrap this part to a JVM. While waiting for an upstream fix, we will probably use this workaround and add the export flags to the gradle.properties for the first rollout of Java 17. Based on that, I'll let the pjf gradle integration use the old codepath and not use a bootstrapped jvm.
|
Let me try deving with the intellij Plugin for a day to see how the latency is. |
| List<URL> implementationUrls = | ||
| getImplementationUrls(cacheKey.implementationClassPath, cacheKey.useBundledImplementation); | ||
| Path jdkPath = getJdkPath(cacheKey.project); | ||
| Integer jdkVersion = getSdkVersion(cacheKey.project); |
There was a problem hiding this comment.
I thought about using the "same-jvm" formatter for older jvm version but given that in intellij we need the bootstrapping formatter for all projects using Java 12+ it would rather just have one code path.
Also the additional overhead shouldn't be as noticeable in intellij as the usual workflow is to just re-format the current file instead of all files in the project.
There was a problem hiding this comment.
I thought about using the "same-jvm" formatter for older jvm version but given that in intellij we need the bootstrapping formatter for all projects using Java 12+ it would rather just have one code path.
It might be helpful to avoid introducing risk/bugs for existing workflows, to begin with anyhow
There was a problem hiding this comment.
I was thinking that ideally we would already use the bootstrapping formatter for Java 15, which basically includes all internal users at this point.
But given that the formatter didn't support new language features on Java 15 so far, we can start defensively with only enabling the bootstrapping formatter for Java 16+ where it would fail to run. Can enable it for Java 15 in a FLUP PR once this has soaked for a while.
|
Reverted the changes to the gradle plugin and fixed the case where we would surface errors of the formatter failing on invalid java syntax to the Intellij UI (we now silently ignore these). Will cut a release now. This effectively just changed the codepath of the Intellij Plugin when using Java 16+ as project SDK. |
|
Released 2.4.0 |
Before this PR
There are two issue with running the formatter targeting newer Java versions:
--add-exportflags due to JEP 396 (ref).After this PR
As a workaround to both problems we bootstrap a new JVM in which we run the formatter. This allows us to specify the additional jvm flags without having to set them on the main JVM process and to run the formatter with a newer JDK than the one provided by Intellij
FLUPS:
Improve performance for spotless-gradle integration- For now we just updated the Intellij plugin.==COMMIT_MSG==
Bootstrap new jvm for running formatter with flags
==COMMIT_MSG==