From f2f59bbd4494f76931e8f57e7276d0ccb3c14523 Mon Sep 17 00:00:00 2001 From: Vladimir Krivosheev Date: Wed, 6 Nov 2019 09:11:02 +0100 Subject: [PATCH] simplify - always pass DescriptorListLoadingContext as not-null GitOrigin-RevId: 54670fcd9c90c34d0bf3e68c71f89449a8efd497 --- .../plugins/DescriptorListLoadingContext.java | 174 +++++++++--------- .../ide/plugins/DescriptorLoadingContext.java | 34 +--- .../ide/plugins/PluginManagerCore.java | 28 ++- .../ide/plugins/PluginManagerTest.java | 8 +- 4 files changed, 117 insertions(+), 127 deletions(-) diff --git a/platform/core-impl/src/com/intellij/ide/plugins/DescriptorListLoadingContext.java b/platform/core-impl/src/com/intellij/ide/plugins/DescriptorListLoadingContext.java index 922cfa552d64..f756ab3a16df 100644 --- a/platform/core-impl/src/com/intellij/ide/plugins/DescriptorListLoadingContext.java +++ b/platform/core-impl/src/com/intellij/ide/plugins/DescriptorListLoadingContext.java @@ -4,7 +4,6 @@ package com.intellij.ide.plugins; import com.intellij.openapi.extensions.PluginId; import com.intellij.openapi.util.SafeJdomFactory; import com.intellij.util.ConcurrencyUtil; -import com.intellij.util.SmartList; import com.intellij.util.concurrency.AppExecutorUtil; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.HashSetInterner; @@ -13,19 +12,24 @@ import org.jdom.*; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.*; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ExecutorService; import java.util.function.Supplier; -final class LoadingDescriptorListContext implements AutoCloseable { +final class DescriptorListLoadingContext implements AutoCloseable { @NotNull - private final ExecutorService myExecutorService; + private final ExecutorService executorService; - private final Collection> myInterners; + private final ConcurrentLinkedQueue> interners; // synchronization will ruin parallel loading, so, string pool is local per thread - private final ThreadLocal myThreadLocalXmlFactory; - private final int myMaxThreads; + private final Supplier xmlFactorySupplier; + @Nullable + private final ThreadLocal threadLocalXmlFactory; + private final int maxThreads; @NotNull final Set disabledPlugins; @@ -41,110 +45,116 @@ final class LoadingDescriptorListContext implements AutoCloseable { return result; }; - LoadingDescriptorListContext(boolean isParallel, @NotNull Set disabledPlugins) { + DescriptorListLoadingContext(boolean isParallel, @NotNull Set disabledPlugins) { this.disabledPlugins = disabledPlugins; - myMaxThreads = isParallel ? (Runtime.getRuntime().availableProcessors() - 1) : 1; - if (myMaxThreads > 1) { - myExecutorService = AppExecutorUtil.createBoundedApplicationPoolExecutor("PluginManager Loader", myMaxThreads, false); - myInterners = Collections.newSetFromMap(ContainerUtil.newConcurrentMap(myMaxThreads)); + maxThreads = isParallel ? (Runtime.getRuntime().availableProcessors() - 1) : 1; + if (maxThreads > 1) { + executorService = AppExecutorUtil.createBoundedApplicationPoolExecutor("PluginManager Loader", maxThreads, false); + interners = new ConcurrentLinkedQueue<>(); + + threadLocalXmlFactory = ThreadLocal.withInitial(() -> { + PluginXmlFactory factory = new PluginXmlFactory(); + interners.add(factory.stringInterner); + return factory; + }); + xmlFactorySupplier = () -> threadLocalXmlFactory.get(); } else { - myExecutorService = ConcurrencyUtil.newSameThreadExecutorService(); - myInterners = new SmartList<>(); + executorService = ConcurrencyUtil.newSameThreadExecutorService(); + interners = null; + threadLocalXmlFactory = null; + xmlFactorySupplier = () -> new PluginXmlFactory(); } - - myThreadLocalXmlFactory = ThreadLocal.withInitial(() -> { - PluginXmlFactory factory = new PluginXmlFactory(); - myInterners.add(factory.stringInterner); - return factory; - }); } @NotNull ExecutorService getExecutorService() { - return myExecutorService; + return executorService; } @NotNull - public SafeJdomFactory getXmlFactory() { - return myThreadLocalXmlFactory.get(); + SafeJdomFactory getXmlFactory() { + return xmlFactorySupplier.get(); } @Override public void close() { - if (myMaxThreads <= 1) { - myThreadLocalXmlFactory.remove(); + if (threadLocalXmlFactory == null) { return; } - myExecutorService.execute(() -> { - for (Interner interner : myInterners) { + if (maxThreads <= 1) { + threadLocalXmlFactory.remove(); + return; + } + + executorService.execute(() -> { + for (Interner interner : interners) { interner.clear(); } }); - myExecutorService.shutdown(); + executorService.shutdown(); + } +} + +/** + * Consider using some threshold in StringInterner - CDATA is not interned at all, + * but maybe some long text for Text node doesn't make sense to intern too. + */ +// don't intern CDATA - in most cases it is used for some unique large text (e.g. plugin description) +final class PluginXmlFactory extends SafeJdomFactory.BaseSafeJdomFactory { + // doesn't make sense to intern class name since it is unique + // ouch, do we really cannot agree how to name implementation class attribute? + private static final List CLASS_NAME_LIST = Arrays.asList( + "implementation-class", "implementation", + "serviceImplementation", "class", "className", "beanClass", + "serviceInterface", "interface", "interfaceClass", "instance", + "qualifiedName"); + + private static final Set CLASS_NAMES = ContainerUtil.newIdentityTroveSet(CLASS_NAME_LIST); + + final Interner stringInterner = new HashSetInterner(ContainerUtil.concat(CLASS_NAME_LIST, IdeaPluginDescriptorImpl.SERVICE_QUALIFIED_ELEMENT_NAMES)) { + @NotNull + @Override + public String intern(@NotNull String name) { + // doesn't make any sense to intern long texts (JdomInternFactory doesn't intern CDATA, but plugin description can be simply Text) + return name.length() < 64 ? super.intern(name) : name; + } + }; + + @NotNull + @Override + public Interner stringInterner() { + return stringInterner; } - /** - * Consider using some threshold in StringInterner - CDATA is not interned at all, - * but maybe some long text for Text node doesn't make sense to intern too. - */ - // don't intern CDATA - in most cases it is used for some unique large text (e.g. plugin description) - private final static class PluginXmlFactory extends SafeJdomFactory.BaseSafeJdomFactory { - // doesn't make sense to intern class name since it is unique - // ouch, do we really cannot agree how to name implementation class attribute? - private static final List CLASS_NAME_LIST = Arrays.asList( - "implementation-class", "implementation", - "serviceImplementation", "class", "className", "beanClass", - "serviceInterface", "interface", "interfaceClass", "instance", - "qualifiedName"); + @NotNull + @Override + public Element element(@NotNull String name, @Nullable Namespace namespace) { + return super.element(stringInterner.intern(name), namespace); + } - private static final Set CLASS_NAMES = ContainerUtil.newIdentityTroveSet(CLASS_NAME_LIST); - - private final Interner - stringInterner = new HashSetInterner(ContainerUtil.concat(CLASS_NAME_LIST, IdeaPluginDescriptorImpl.SERVICE_QUALIFIED_ELEMENT_NAMES)) { - @NotNull - @Override - public String intern(@NotNull String name) { - // doesn't make any sense to intern long texts (JdomInternFactory doesn't intern CDATA, but plugin description can be simply Text) - return name.length() < 64 ? super.intern(name) : name; - } - }; - - @NotNull - @Override - public Interner stringInterner() { - return stringInterner; + @NotNull + @Override + public Attribute attribute(@NotNull String name, @NotNull String value, @Nullable AttributeType type, @Nullable Namespace namespace) { + String internedName = stringInterner.intern(name); + if (CLASS_NAMES.contains(internedName)) { + return super.attribute(internedName, value, type, namespace); } - - @NotNull - @Override - public Element element(@NotNull String name, @Nullable Namespace namespace) { - return super.element(stringInterner.intern(name), namespace); + else { + return super.attribute(internedName, stringInterner.intern(value), type, namespace); } + } - @NotNull - @Override - public Attribute attribute(@NotNull String name, @NotNull String value, @Nullable AttributeType type, @Nullable Namespace namespace) { - String internedName = stringInterner.intern(name); - if (CLASS_NAMES.contains(internedName)) { - return super.attribute(internedName, value, type, namespace); - } - else { - return super.attribute(internedName, stringInterner.intern(value), type, namespace); - } + @NotNull + @Override + public Text text(@NotNull String text, @NotNull Element parentElement) { + if (CLASS_NAMES.contains(parentElement.getName())) { + return super.text(text, parentElement); } - - @NotNull - @Override - public Text text(@NotNull String text, @NotNull Element parentElement) { - if (CLASS_NAMES.contains(parentElement.getName())) { - return super.text(text, parentElement); - } - else { - return super.text(stringInterner.intern(text), parentElement); - } + else { + return super.text(stringInterner.intern(text), parentElement); } } } diff --git a/platform/core-impl/src/com/intellij/ide/plugins/DescriptorLoadingContext.java b/platform/core-impl/src/com/intellij/ide/plugins/DescriptorLoadingContext.java index 385099f39634..89fee979597f 100644 --- a/platform/core-impl/src/com/intellij/ide/plugins/DescriptorLoadingContext.java +++ b/platform/core-impl/src/com/intellij/ide/plugins/DescriptorLoadingContext.java @@ -1,28 +1,23 @@ // Copyright 2000-2019 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. package com.intellij.ide.plugins; -import com.intellij.openapi.extensions.PluginId; import com.intellij.openapi.util.SafeJdomFactory; -import com.intellij.util.containers.HashSetInterner; import com.intellij.util.containers.Interner; import gnu.trove.THashMap; import org.jdom.Element; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; import java.io.IOException; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Path; import java.util.*; -import java.util.function.Supplier; final class DescriptorLoadingContext implements AutoCloseable { final Map openedFiles = new THashMap<>(); - final LoadingDescriptorListContext parentContext; + final DescriptorListLoadingContext parentContext; final boolean isBundled; final boolean isEssential; - final Set disabledPlugins; private List> visitedFiles; @@ -33,15 +28,13 @@ final class DescriptorLoadingContext implements AutoCloseable { /** * parentContext is null only for CoreApplicationEnvironment - it is not valid otherwise because in this case XML is not interned. */ - DescriptorLoadingContext(@Nullable LoadingDescriptorListContext parentContext, + DescriptorLoadingContext(@NotNull DescriptorListLoadingContext parentContext, boolean isBundled, boolean isEssential, - @NotNull Set disabledPlugins, @NotNull PathBasedJdomXIncluder.PathResolver pathResolver) { this.parentContext = parentContext; this.isBundled = isBundled; this.isEssential = isEssential; - this.disabledPlugins = disabledPlugins; this.pathResolver = pathResolver; } @@ -65,9 +58,9 @@ final class DescriptorLoadingContext implements AutoCloseable { return result; } - @Nullable + @NotNull SafeJdomFactory getXmlFactory() { - return parentContext == null ? null : parentContext.getXmlFactory(); + return parentContext.getXmlFactory(); } @Override @@ -83,26 +76,15 @@ final class DescriptorLoadingContext implements AutoCloseable { @NotNull public DescriptorLoadingContext copy(boolean isEssential) { - return new DescriptorLoadingContext(parentContext, isBundled, isEssential, disabledPlugins, pathResolver); + return new DescriptorLoadingContext(parentContext, isBundled, isEssential, pathResolver); } void readDescriptor(@NotNull IdeaPluginDescriptorImpl descriptor, @NotNull Element element, @NotNull Path basePath, @NotNull PathBasedJdomXIncluder.PathResolver resolver) { - LoadingDescriptorListContext parentContext = this.parentContext; - Supplier defaultVersion; - Interner stringInterner; - if (parentContext == null) { - defaultVersion = IdeaPluginDescriptorImpl.DEFAULT_VERSION_SUPPLIER; - stringInterner = new HashSetInterner<>(); - } - else { - defaultVersion = parentContext.defaultVersionSupplier; - // always PluginXmlFactory with not-null interner - stringInterner = Objects.requireNonNull(parentContext.getXmlFactory().stringInterner()); - } - - descriptor.readExternal(element, basePath, PluginManagerCore.isUnitTestMode, resolver, stringInterner, disabledPlugins, defaultVersion); + // always PluginXmlFactory with not-null interner + Interner stringInterner = Objects.requireNonNull(parentContext.getXmlFactory().stringInterner()); + descriptor.readExternal(element, basePath, PluginManagerCore.isUnitTestMode, resolver, stringInterner, parentContext.disabledPlugins, parentContext.defaultVersionSupplier); } } diff --git a/platform/core-impl/src/com/intellij/ide/plugins/PluginManagerCore.java b/platform/core-impl/src/com/intellij/ide/plugins/PluginManagerCore.java index 681da35b8d8f..5f2ddc3b1bf1 100644 --- a/platform/core-impl/src/com/intellij/ide/plugins/PluginManagerCore.java +++ b/platform/core-impl/src/com/intellij/ide/plugins/PluginManagerCore.java @@ -878,7 +878,8 @@ public final class PluginManagerCore { @Nullable public static IdeaPluginDescriptorImpl loadDescriptor(@NotNull Path file, @NotNull String fileName, @Nullable Set disabledPlugins) { - try (DescriptorLoadingContext context = new DescriptorLoadingContext(null, false, false, disabledPlugins == null ? Collections.emptySet() : disabledPlugins, + Set disabled = disabledPlugins == null ? Collections.emptySet() : disabledPlugins; + try (DescriptorLoadingContext context = new DescriptorLoadingContext(new DescriptorListLoadingContext(false, disabled), false, false, PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER)) { return loadDescriptorFromFileOrDir(file, fileName, context, Files.isDirectory(file)); } @@ -887,9 +888,8 @@ public final class PluginManagerCore { @Nullable private static IdeaPluginDescriptorImpl loadDescriptor(@NotNull Path file, boolean isBundled, - @NotNull Set disabledPlugins, - @Nullable LoadingDescriptorListContext parentContext) { - try (DescriptorLoadingContext context = new DescriptorLoadingContext(parentContext, isBundled, /* isEssential = */ false, disabledPlugins, + @NotNull DescriptorListLoadingContext parentContext) { + try (DescriptorLoadingContext context = new DescriptorLoadingContext(parentContext, isBundled, /* isEssential = */ false, PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER)) { return loadDescriptorFromFileOrDir(file, PLUGIN_XML, context, Files.isDirectory(file)); } @@ -1100,12 +1100,12 @@ public final class PluginManagerCore { private static void loadDescriptorsFromDir(@NotNull Path dir, @NotNull PluginLoadingResult result, boolean isBundled, - @NotNull LoadingDescriptorListContext context) throws ExecutionException, InterruptedException { + @NotNull DescriptorListLoadingContext context) throws ExecutionException, InterruptedException { List> tasks = new ArrayList<>(); ExecutorService executorService = context.getExecutorService(); try (DirectoryStream dirStream = Files.newDirectoryStream(dir)) { for (Path file : dirStream) { - tasks.add(executorService.submit(() -> loadDescriptor(file, isBundled, context.disabledPlugins, context))); + tasks.add(executorService.submit(() -> loadDescriptor(file, isBundled, context))); } } catch (IOException ignore) { @@ -1162,7 +1162,7 @@ public final class PluginManagerCore { PluginLoadingResult result = new PluginLoadingResult(Collections.emptyMap()); LinkedHashMap urlsFromClassPath = new LinkedHashMap<>(); URL platformPluginURL = computePlatformPluginUrlAndCollectPluginUrls(loader, urlsFromClassPath); - try (DescriptorLoadingContext loadingContext = new DescriptorLoadingContext(new LoadingDescriptorListContext(false, Collections.emptySet()), true, true, Collections.emptySet(), new ClassPathXmlPathResolver(loader))) { + try (DescriptorLoadingContext loadingContext = new DescriptorLoadingContext(new DescriptorListLoadingContext(false, Collections.emptySet()), true, true, new ClassPathXmlPathResolver(loader))) { loadDescriptorsFromClassPath(urlsFromClassPath, result, loadingContext, platformPluginURL); } result.finish(); @@ -1173,7 +1173,7 @@ public final class PluginManagerCore { public static List testLoadDescriptorsFromDir(@NotNull Path dir) throws ExecutionException, InterruptedException { PluginLoadingResult result = new PluginLoadingResult(Collections.emptyMap()); - loadDescriptorsFromDir(dir, result, true, new LoadingDescriptorListContext(false, Collections.emptySet())); + loadDescriptorsFromDir(dir, result, true, new DescriptorListLoadingContext(false, Collections.emptySet())); result.finish(); return result.plugins; } @@ -1288,16 +1288,15 @@ public final class PluginManagerCore { } private static void loadDescriptorsFromProperty(@NotNull PluginLoadingResult result, - @NotNull LoadingDescriptorListContext context) { + @NotNull DescriptorListLoadingContext context) { String pathProperty = System.getProperty(PROPERTY_PLUGIN_PATH); if (pathProperty == null) { return; } - Set disabledPlugins = disabledPlugins(); for (StringTokenizer t = new StringTokenizer(pathProperty, File.pathSeparator + ","); t.hasMoreTokens(); ) { String s = t.nextToken(); - IdeaPluginDescriptorImpl descriptor = loadDescriptor(Paths.get(s), false, disabledPlugins, context); + IdeaPluginDescriptorImpl descriptor = loadDescriptor(Paths.get(s), false, context); if (descriptor != null) { result.add(descriptor, /* silentlyIgnoreIfDuplicate = */ false); } @@ -1321,9 +1320,8 @@ public final class PluginManagerCore { ClassLoader classLoader = PluginManagerCore.class.getClassLoader(); URL platformPluginURL = computePlatformPluginUrlAndCollectPluginUrls(classLoader, urlsFromClassPath); boolean parallel = SystemProperties.getBooleanProperty("parallel.pluginDescriptors.loading", true); - try (LoadingDescriptorListContext context = new LoadingDescriptorListContext(parallel, disabledPlugins())) { - try (DescriptorLoadingContext loadingContext = new DescriptorLoadingContext(context, /* isBundled = */ true, /* isEssential, doesn't matter = */ true, - context.disabledPlugins, new ClassPathXmlPathResolver(classLoader))) { + try (DescriptorListLoadingContext context = new DescriptorListLoadingContext(parallel, disabledPlugins())) { + try (DescriptorLoadingContext loadingContext = new DescriptorLoadingContext(context, /* isBundled = */ true, /* isEssential, doesn't matter = */ true, new ClassPathXmlPathResolver(classLoader))) { loadDescriptorsFromClassPath(urlsFromClassPath, result, loadingContext, platformPluginURL); } @@ -1885,7 +1883,7 @@ public final class PluginManagerCore { */ public static void registerExtensionPointAndExtensions(@NotNull Path pluginRoot, @NotNull String fileName, @NotNull ExtensionsArea area) { IdeaPluginDescriptorImpl descriptor; - try (DescriptorLoadingContext context = new DescriptorLoadingContext(null, true, true, disabledPlugins(), PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER)) { + try (DescriptorLoadingContext context = new DescriptorLoadingContext(new DescriptorListLoadingContext(false, disabledPlugins()), true, true, PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER)) { if (Files.isDirectory(pluginRoot)) { descriptor = loadDescriptorFromDir(pluginRoot, fileName, null, context); } diff --git a/platform/platform-tests/testSrc/com/intellij/ide/plugins/PluginManagerTest.java b/platform/platform-tests/testSrc/com/intellij/ide/plugins/PluginManagerTest.java index 515d8b835a9a..efdcd1fc4909 100644 --- a/platform/platform-tests/testSrc/com/intellij/ide/plugins/PluginManagerTest.java +++ b/platform/platform-tests/testSrc/com/intellij/ide/plugins/PluginManagerTest.java @@ -147,14 +147,14 @@ public class PluginManagerTest { throws IOException, JDOMException { Path file = Paths.get(getTestDataPath(), testDataName); PluginLoadingResult result = new PluginLoadingResult(Collections.emptyMap()); - LoadingDescriptorListContext context = new LoadingDescriptorListContext(false, Collections.emptySet()); + DescriptorListLoadingContext context = new DescriptorListLoadingContext(false, Collections.emptySet()); Element root = JDOMUtil.load(file, context.getXmlFactory()); for (Element element : root.getChildren("idea-plugin")) { String url = element.getAttributeValue("url"); - IdeaPluginDescriptorImpl d = new IdeaPluginDescriptorImpl(Paths.get(url), true); - d.readExternal(element, Paths.get(url), true, PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER, + IdeaPluginDescriptorImpl descriptor = new IdeaPluginDescriptorImpl(Paths.get(url), true); + descriptor.readExternal(element, Paths.get(url), true, PathBasedJdomXIncluder.DEFAULT_PATH_RESOLVER, context.getXmlFactory().stringInterner(), Collections.emptySet(), DEFAULT_VERSION_SUPPLIER); - result.add(d, false); + result.add(descriptor, false); } result.finish(); return result;