-
Notifications
You must be signed in to change notification settings - Fork 49
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
Bootstrap new jvm for running formatter with flags #562
Changes from 4 commits
e859a41
6d6bfa2
96aabfa
0505880
11ecb0e
2d0c369
550a8fe
af74d8a
072765d
c4cf445
b47a197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
/* | ||
* (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.palantir.javaformat.intellij; | ||
|
||
import com.github.benmanes.caffeine.cache.Caffeine; | ||
import com.github.benmanes.caffeine.cache.LoadingCache; | ||
import com.intellij.openapi.project.Project; | ||
import com.intellij.openapi.projectRoots.JdkUtil; | ||
import com.intellij.openapi.projectRoots.Sdk; | ||
import com.intellij.openapi.roots.ProjectRootManager; | ||
import com.palantir.javaformat.bootstrap.BootstrappingFormatterService; | ||
import com.palantir.javaformat.java.FormatterService; | ||
import com.palantir.logsafe.Preconditions; | ||
import java.io.IOException; | ||
import java.net.MalformedURLException; | ||
import java.net.URI; | ||
import java.net.URL; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.Optional; | ||
import java.util.jar.Attributes.Name; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
final class FormatterProvider { | ||
private static final Logger log = LoggerFactory.getLogger(FormatterProvider.class); | ||
|
||
// Cache to avoid creating a URLClassloader every time we want to format from IntelliJ | ||
private final LoadingCache<FormatterCacheKey, FormatterService> implementationCache = | ||
Caffeine.newBuilder().maximumSize(1).build(FormatterProvider::createFormatter); | ||
|
||
FormatterService get(Project project, PalantirJavaFormatSettings settings) { | ||
return implementationCache.get(new FormatterCacheKey( | ||
project, settings.getImplementationClassPath(), settings.injectedVersionIsOutdated())); | ||
} | ||
|
||
private static FormatterService createFormatter(FormatterCacheKey cacheKey) { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It might be helpful to avoid introducing risk/bugs for existing workflows, to begin with anyhow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
log.debug("Running formatter with jdk version {} and path: {}", jdkVersion, jdkPath); | ||
return new BootstrappingFormatterService(jdkPath, jdkVersion, implementationUrls); | ||
} | ||
|
||
private static List<URL> getProvidedImplementationUrls(List<URI> implementationClasspath) { | ||
return implementationClasspath.stream() | ||
.map(FormatterProvider::toUrlUnchecked) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
private static List<URL> getBundledImplementationUrls() { | ||
// Load from the jars bundled with the plugin. | ||
Path implDir = PalantirCodeStyleManager.PLUGIN.getPath().toPath().resolve("impl"); | ||
log.debug("Using palantir-java-format implementation bundled with plugin: {}", implDir); | ||
return listDirAsUrlsUnchecked(implDir); | ||
} | ||
|
||
private static List<URL> getImplementationUrls( | ||
Optional<List<URI>> implementationClassPath, boolean useBundledImplementation) { | ||
if (useBundledImplementation) { | ||
log.debug("Using palantir-java-format implementation bundled with plugin"); | ||
return getBundledImplementationUrls(); | ||
} | ||
return implementationClassPath | ||
.map(classpath -> { | ||
log.debug("Using palantir-java-format implementation defined by URIs: {}", classpath); | ||
return getProvidedImplementationUrls(classpath); | ||
}) | ||
.orElseGet(() -> { | ||
log.debug("Using palantir-java-format implementation bundled with plugin"); | ||
return getBundledImplementationUrls(); | ||
}); | ||
} | ||
|
||
private static Path getJdkPath(Project project) { | ||
return getProjectJdk(project) | ||
.map(Sdk::getHomePath) | ||
.map(Path::of) | ||
.map(sdkHome -> sdkHome.resolve("bin").resolve("java")) | ||
.filter(Files::exists) | ||
.orElseThrow(() -> new IllegalStateException("Could not determine jdk path for project " + project)); | ||
} | ||
|
||
private static Integer getSdkVersion(Project project) { | ||
return getProjectJdk(project) | ||
.map(FormatterProvider::parseSdkJavaVersion) | ||
.orElseThrow(() -> new IllegalStateException("Could not determine jdk version for project " + project)); | ||
} | ||
|
||
private static Integer parseSdkJavaVersion(Sdk sdk) { | ||
// Parses the actual version out of "SDK#getVersionString" which returns 'java version "15"'. | ||
String version = Preconditions.checkNotNull( | ||
JdkUtil.getJdkMainAttribute(sdk, Name.IMPLEMENTATION_VERSION), "JDK version is null"); | ||
try { | ||
return Integer.parseInt(version); | ||
} catch (NumberFormatException e) { | ||
log.error("Could not parse sdk version: {}", version, e); | ||
return null; | ||
} | ||
} | ||
|
||
private static Optional<Sdk> getProjectJdk(Project project) { | ||
return Optional.ofNullable(ProjectRootManager.getInstance(project).getProjectSdk()); | ||
} | ||
|
||
private static URL toUrlUnchecked(URI uri) { | ||
try { | ||
return uri.toURL(); | ||
} catch (IllegalArgumentException | MalformedURLException e) { | ||
throw new RuntimeException("Couldn't convert URI to URL: " + uri, e); | ||
} | ||
} | ||
|
||
private static List<URL> listDirAsUrlsUnchecked(Path dir) { | ||
try (Stream<Path> list = Files.list(dir)) { | ||
return list.map(Path::toUri).map(FormatterProvider::toUrlUnchecked).collect(Collectors.toList()); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Couldn't list dir: " + dir, e); | ||
} | ||
} | ||
|
||
private static final class FormatterCacheKey { | ||
private final Project project; | ||
private final Optional<List<URI>> implementationClassPath; | ||
private final boolean useBundledImplementation; | ||
|
||
FormatterCacheKey( | ||
Project project, Optional<List<URI>> implementationClassPath, boolean useBundledImplementation) { | ||
this.project = project; | ||
this.implementationClassPath = implementationClassPath; | ||
this.useBundledImplementation = useBundledImplementation; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
FormatterCacheKey that = (FormatterCacheKey) o; | ||
return useBundledImplementation == that.useBundledImplementation | ||
&& project.equals(that.project) | ||
&& implementationClassPath.equals(that.implementationClassPath); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(project, implementationClassPath, useBundledImplementation); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.