From 5934166df704ddab743bb2a390ad29dafa17b537 Mon Sep 17 00:00:00 2001 From: Kirill Likhodedov Date: Fri, 15 May 2015 18:26:13 +0300 Subject: [PATCH] [log] Don't call Disposer.register from VisiblePackBuilderTest Since nobody calls dispose afterwards => a leak happens. For this extract the DataGetter interface and use it in the VisiblePackBuilder. Provide a stub implementation in the test. --- .../vcs/log/data/AbstractDataGetter.java | 204 +++++++++++++++++ .../vcs/log/data/CommitDetailsGetter.java | 2 +- .../com/intellij/vcs/log/data/DataGetter.java | 211 ++---------------- .../vcs/log/data/MiniDetailsGetter.java | 2 +- .../vcs/log/data/VisiblePackBuilder.java | 4 +- .../vcs/log/data/VisiblePackBuilderTest.kt | 63 +++--- 6 files changed, 255 insertions(+), 231 deletions(-) create mode 100644 platform/vcs-log/impl/src/com/intellij/vcs/log/data/AbstractDataGetter.java diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/AbstractDataGetter.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/AbstractDataGetter.java new file mode 100644 index 000000000000..6a21ad975cd3 --- /dev/null +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/AbstractDataGetter.java @@ -0,0 +1,204 @@ +package com.intellij.vcs.log.data; + +import com.intellij.openapi.Disposable; +import com.intellij.openapi.util.Disposer; +import com.intellij.openapi.vcs.VcsException; +import com.intellij.openapi.vfs.VirtualFile; +import com.intellij.util.Function; +import com.intellij.util.ThrowableConsumer; +import com.intellij.util.containers.ContainerUtil; +import com.intellij.util.containers.MultiMap; +import com.intellij.util.ui.UIUtil; +import com.intellij.vcs.log.VcsLogHashMap; +import com.intellij.vcs.log.VcsLogProvider; +import com.intellij.vcs.log.VcsShortCommitDetails; +import com.intellij.vcs.log.ui.tables.GraphTableModel; +import com.intellij.vcs.log.util.SequentialLimitedLifoExecutor; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.awt.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Map; + +/** + * The DataGetter realizes the following pattern of getting some data (parametrized by {@code T}) from the VCS: + * + * + * @author Kirill Likhodedov + */ +abstract class AbstractDataGetter implements Disposable, DataGetter { + + private static final int UP_PRELOAD_COUNT = 20; + private static final int DOWN_PRELOAD_COUNT = 40; + private static final int MAX_LOADING_TASKS = 10; + + @NotNull protected final VcsLogHashMap myHashMap; + @NotNull private final Map myLogProviders; + @NotNull private final VcsCommitCache myCache; + @NotNull private final SequentialLimitedLifoExecutor myLoader; + + /** + * The sequence number of the current "loading" task. + */ + private long myCurrentTaskIndex = 0; + + @NotNull private final Collection myLoadingFinishedListeners = new ArrayList(); + + AbstractDataGetter(@NotNull VcsLogHashMap hashMap, + @NotNull Map logProviders, + @NotNull VcsCommitCache cache, + @NotNull Disposable parentDisposable) { + myHashMap = hashMap; + myLogProviders = logProviders; + myCache = cache; + Disposer.register(parentDisposable, this); + myLoader = new SequentialLimitedLifoExecutor(this, MAX_LOADING_TASKS, + new ThrowableConsumer() { + @Override + public void consume(TaskDescriptor task) throws VcsException { + preLoadCommitData(task.myCommits); + UIUtil.invokeAndWaitIfNeeded(new Runnable() { + @Override + public void run() { + for (Runnable loadingFinishedListener : myLoadingFinishedListeners) { + loadingFinishedListener.run(); + } + } + }); + } + }); + } + + @Override + public void dispose() { + myLoadingFinishedListeners.clear(); + } + + @Override + @Nullable + public T getCommitData(int row, @NotNull GraphTableModel tableModel) { + assert EventQueue.isDispatchThread(); + Integer hash = tableModel.getCommitIdAtRow(row); + T details = getFromCache(hash); + if (details != null) { + return details; + } + runLoadAroundCommitData(row, tableModel); + return myCache.get(hash); // now it is in the cache as "Loading Details". + } + + @Override + @Nullable + public T getCommitDataIfAvailable(int hash) { + return getFromCache(hash); + } + + @Nullable + private T getFromCache(@NotNull Integer commitId) { + T details = myCache.get(commitId); + if (details != null) { + if (details instanceof LoadingDetails) { + if (((LoadingDetails)details).getLoadingTaskIndex() <= myCurrentTaskIndex - MAX_LOADING_TASKS) { + // don't let old "loading" requests stay in the cache forever + myCache.remove(commitId); + return null; + } + } + return details; + } + return getFromAdditionalCache(commitId); + } + + /** + * Lookup somewhere else but the standard cache. + */ + @Nullable + protected abstract T getFromAdditionalCache(int commitId); + + private void runLoadAroundCommitData(int row, @NotNull GraphTableModel tableModel) { + long taskNumber = myCurrentTaskIndex++; + MultiMap commits = getCommitsAround(row, tableModel, UP_PRELOAD_COUNT, DOWN_PRELOAD_COUNT); + for (Map.Entry> hashesByRoots : commits.entrySet()) { + VirtualFile root = hashesByRoots.getKey(); + Collection hashes = hashesByRoots.getValue(); + + // fill the cache with temporary "Loading" values to avoid producing queries for each commit that has not been cached yet, + // even if it will be loaded within a previous query + for (int commitId : hashes) { + if (!myCache.isKeyCached(commitId)) { + myCache.put(commitId, (T)new LoadingDetails(myHashMap.getHash(commitId), taskNumber, root)); + } + } + } + + TaskDescriptor task = new TaskDescriptor(commits); + myLoader.queue(task); + } + + @NotNull + private static MultiMap getCommitsAround(int selectedRow, + @NotNull GraphTableModel model, + int above, + int below) { + MultiMap commits = MultiMap.create(); + for (int row = Math.max(0, selectedRow - above); row < selectedRow + below && row < model.getRowCount(); row++) { + Integer hash = model.getCommitIdAtRow(row); + VirtualFile root = model.getRoot(row); + commits.putValue(root, hash); + } + return commits; + } + + private void preLoadCommitData(@NotNull MultiMap commits) throws VcsException { + for (Map.Entry> entry : commits.entrySet()) { + List hashStrings = ContainerUtil.map(entry.getValue(), new Function() { + @Override + public String fun(Integer commitId) { + return myHashMap.getHash(commitId).asString(); + } + }); + List details = readDetails(myLogProviders.get(entry.getKey()), entry.getKey(), hashStrings); + saveInCache(details); + } + } + + public void saveInCache(final List details) { + UIUtil.invokeAndWaitIfNeeded(new Runnable() { + @Override + public void run() { + for (T data : details) { + myCache.put(myHashMap.getCommitIndex(data.getId()), data); + } + } + }); + } + + @NotNull + protected abstract List readDetails(@NotNull VcsLogProvider logProvider, @NotNull VirtualFile root, + @NotNull List hashes) throws VcsException; + + /** + * This listener will be notified when any details loading process finishes. + * The notification will happen in the EDT. + */ + public void addDetailsLoadedListener(@NotNull Runnable runnable) { + myLoadingFinishedListeners.add(runnable); + } + + private static class TaskDescriptor { + private final MultiMap myCommits; + + private TaskDescriptor(MultiMap commits) { + myCommits = commits; + } + } + +} diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/CommitDetailsGetter.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/CommitDetailsGetter.java index 7e1d44c81efe..e2a331f07805 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/CommitDetailsGetter.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/CommitDetailsGetter.java @@ -15,7 +15,7 @@ import java.util.Map; /** * The CommitDetailsGetter is responsible for getting {@link VcsFullCommitDetails complete commit details} from the cache or from the VCS. */ -public class CommitDetailsGetter extends DataGetter { +public class CommitDetailsGetter extends AbstractDataGetter { CommitDetailsGetter(@NotNull VcsLogHashMap hashMap, @NotNull Map logProviders, diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/DataGetter.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/DataGetter.java index d6c508d38490..4da4b90a5ed7 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/DataGetter.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/DataGetter.java @@ -1,202 +1,29 @@ +/* + * Copyright 2000-2015 JetBrains s.r.o. + * + * 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.intellij.vcs.log.data; -import com.intellij.openapi.Disposable; -import com.intellij.openapi.util.Disposer; -import com.intellij.openapi.vcs.VcsException; -import com.intellij.openapi.vfs.VirtualFile; -import com.intellij.util.Function; -import com.intellij.util.ThrowableConsumer; -import com.intellij.util.containers.ContainerUtil; -import com.intellij.util.containers.MultiMap; -import com.intellij.util.ui.UIUtil; -import com.intellij.vcs.log.VcsLogHashMap; -import com.intellij.vcs.log.VcsLogProvider; import com.intellij.vcs.log.VcsShortCommitDetails; import com.intellij.vcs.log.ui.tables.GraphTableModel; -import com.intellij.vcs.log.util.SequentialLimitedLifoExecutor; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.awt.*; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Map; - -/** - * The DataGetter realizes the following pattern of getting some data (parametrized by {@code T}) from the VCS: - *
    - *
  • it tries to get it from the cache;
    • - *
  • if it fails, it tries to get it from the VCS, and additionally loads several commits around the requested one, - * to avoid querying the VCS if user investigates details of nearby commits.
    • - *
  • The loading happens asynchronously: a fake {@link LoadingDetails} object is returned
    • - *
- * - * @author Kirill Likhodedov - */ -public abstract class DataGetter implements Disposable { - - private static final int UP_PRELOAD_COUNT = 20; - private static final int DOWN_PRELOAD_COUNT = 40; - private static final int MAX_LOADING_TASKS = 10; - - @NotNull protected final VcsLogHashMap myHashMap; - @NotNull private final Map myLogProviders; - @NotNull private final VcsCommitCache myCache; - @NotNull private final SequentialLimitedLifoExecutor myLoader; - - /** - * The sequence number of the current "loading" task. - */ - private long myCurrentTaskIndex = 0; - - @NotNull private final Collection myLoadingFinishedListeners = new ArrayList(); - - DataGetter(@NotNull VcsLogHashMap hashMap, - @NotNull Map logProviders, - @NotNull VcsCommitCache cache, - @NotNull Disposable parentDisposable) { - myHashMap = hashMap; - myLogProviders = logProviders; - myCache = cache; - Disposer.register(parentDisposable, this); - myLoader = new SequentialLimitedLifoExecutor(this, MAX_LOADING_TASKS, - new ThrowableConsumer() { - @Override - public void consume(TaskDescriptor task) throws VcsException { - preLoadCommitData(task.myCommits); - UIUtil.invokeAndWaitIfNeeded(new Runnable() { - @Override - public void run() { - for (Runnable loadingFinishedListener : myLoadingFinishedListeners) { - loadingFinishedListener.run(); - } - } - }); - } - }); - } - - @Override - public void dispose() { - myLoadingFinishedListeners.clear(); - } +public interface DataGetter { + @Nullable + T getCommitData(int row, @NotNull GraphTableModel tableModel); @Nullable - public T getCommitData(int row, @NotNull GraphTableModel tableModel) { - assert EventQueue.isDispatchThread(); - Integer hash = tableModel.getCommitIdAtRow(row); - T details = getFromCache(hash); - if (details != null) { - return details; - } - runLoadAroundCommitData(row, tableModel); - return myCache.get(hash); // now it is in the cache as "Loading Details". - } - - @Nullable - public T getCommitDataIfAvailable(int hash) { - return getFromCache(hash); - } - - @Nullable - private T getFromCache(@NotNull Integer commitId) { - T details = myCache.get(commitId); - if (details != null) { - if (details instanceof LoadingDetails) { - if (((LoadingDetails)details).getLoadingTaskIndex() <= myCurrentTaskIndex - MAX_LOADING_TASKS) { - // don't let old "loading" requests stay in the cache forever - myCache.remove(commitId); - return null; - } - } - return details; - } - return getFromAdditionalCache(commitId); - } - - /** - * Lookup somewhere else but the standard cache. - */ - @Nullable - protected abstract T getFromAdditionalCache(int commitId); - - private void runLoadAroundCommitData(int row, @NotNull GraphTableModel tableModel) { - long taskNumber = myCurrentTaskIndex++; - MultiMap commits = getCommitsAround(row, tableModel, UP_PRELOAD_COUNT, DOWN_PRELOAD_COUNT); - for (Map.Entry> hashesByRoots : commits.entrySet()) { - VirtualFile root = hashesByRoots.getKey(); - Collection hashes = hashesByRoots.getValue(); - - // fill the cache with temporary "Loading" values to avoid producing queries for each commit that has not been cached yet, - // even if it will be loaded within a previous query - for (int commitId : hashes) { - if (!myCache.isKeyCached(commitId)) { - myCache.put(commitId, (T)new LoadingDetails(myHashMap.getHash(commitId), taskNumber, root)); - } - } - } - - TaskDescriptor task = new TaskDescriptor(commits); - myLoader.queue(task); - } - - @NotNull - private static MultiMap getCommitsAround(int selectedRow, - @NotNull GraphTableModel model, - int above, - int below) { - MultiMap commits = MultiMap.create(); - for (int row = Math.max(0, selectedRow - above); row < selectedRow + below && row < model.getRowCount(); row++) { - Integer hash = model.getCommitIdAtRow(row); - VirtualFile root = model.getRoot(row); - commits.putValue(root, hash); - } - return commits; - } - - private void preLoadCommitData(@NotNull MultiMap commits) throws VcsException { - for (Map.Entry> entry : commits.entrySet()) { - List hashStrings = ContainerUtil.map(entry.getValue(), new Function() { - @Override - public String fun(Integer commitId) { - return myHashMap.getHash(commitId).asString(); - } - }); - List details = readDetails(myLogProviders.get(entry.getKey()), entry.getKey(), hashStrings); - saveInCache(details); - } - } - - public void saveInCache(final List details) { - UIUtil.invokeAndWaitIfNeeded(new Runnable() { - @Override - public void run() { - for (T data : details) { - myCache.put(myHashMap.getCommitIndex(data.getId()), data); - } - } - }); - } - - @NotNull - protected abstract List readDetails(@NotNull VcsLogProvider logProvider, @NotNull VirtualFile root, - @NotNull List hashes) throws VcsException; - - /** - * This listener will be notified when any details loading process finishes. - * The notification will happen in the EDT. - */ - public void addDetailsLoadedListener(@NotNull Runnable runnable) { - myLoadingFinishedListeners.add(runnable); - } - - private static class TaskDescriptor { - private final MultiMap myCommits; - - private TaskDescriptor(MultiMap commits) { - myCommits = commits; - } - } - + T getCommitDataIfAvailable(int hash); } diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/MiniDetailsGetter.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/MiniDetailsGetter.java index 7a54b261749c..1c63f4c762a7 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/MiniDetailsGetter.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/MiniDetailsGetter.java @@ -13,7 +13,7 @@ import org.jetbrains.annotations.Nullable; import java.util.List; import java.util.Map; -public class MiniDetailsGetter extends DataGetter { +public class MiniDetailsGetter extends AbstractDataGetter { @NotNull private final Map myTopCommitsDetailsCache; diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VisiblePackBuilder.java b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VisiblePackBuilder.java index f3c9afc873b0..9759fde59aa6 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VisiblePackBuilder.java +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/data/VisiblePackBuilder.java @@ -42,13 +42,13 @@ class VisiblePackBuilder { @NotNull private final VcsLogHashMap myHashMap; @NotNull private final Map myTopCommitsDetailsCache; - @NotNull private final CommitDetailsGetter myCommitDetailsGetter; + @NotNull private final DataGetter myCommitDetailsGetter; @NotNull private final Map myLogProviders; VisiblePackBuilder(@NotNull Map providers, @NotNull VcsLogHashMap hashMap, @NotNull Map topCommitsDetailsCache, - @NotNull CommitDetailsGetter detailsGetter) { + @NotNull DataGetter detailsGetter) { myHashMap = hashMap; myTopCommitsDetailsCache = topCommitsDetailsCache; myCommitDetailsGetter = detailsGetter; diff --git a/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VisiblePackBuilderTest.kt b/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VisiblePackBuilderTest.kt index d18d2358248f..b673578c931d 100644 --- a/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VisiblePackBuilderTest.kt +++ b/platform/vcs-log/impl/test/com/intellij/vcs/log/data/VisiblePackBuilderTest.kt @@ -15,37 +15,26 @@ */ package com.intellij.vcs.log.data -import org.junit.Test import com.intellij.mock.MockVirtualFile -import com.intellij.vcs.log.impl.TestVcsLogProvider -import com.intellij.vcs.log.impl.TestVcsLogProvider.* -import com.intellij.vcs.log.VcsLogHashMap -import com.intellij.openapi.vfs.VirtualFile import com.intellij.openapi.Disposable -import com.intellij.vcs.log.graph.PermanentGraph -import com.intellij.vcs.log.impl.VcsLogFilterCollectionImpl -import com.intellij.vcs.log.Hash -import com.intellij.vcs.log.impl.HashImpl -import java.util.HashMap -import com.intellij.vcs.log.graph.GraphCommit -import java.util.ArrayList -import com.intellij.vcs.log.graph.GraphCommitImpl -import com.intellij.vcs.log.VcsRef -import java.util.HashSet -import com.intellij.vcs.log.impl.VcsRefImpl -import com.intellij.vcs.log.VcsLogFilterCollection -import kotlin.test.assertEquals -import com.intellij.vcs.log.VcsLogUserFilter -import com.intellij.vcs.log.impl.VcsCommitMetadataImpl -import com.intellij.vcs.log.VcsUser -import com.intellij.vcs.log.impl.VcsUserImpl -import com.intellij.vcs.log.ui.filter.VcsLogUserFilterImpl -import kotlin.test.assertTrue -import com.intellij.vcs.log.graph.VisibleGraph -import com.intellij.vcs.log.VcsLogBranchFilter -import com.intellij.vcs.log.TimedVcsCommit +import com.intellij.openapi.vfs.VirtualFile import com.intellij.util.Function -import com.intellij.vcs.log.impl.TimedVcsCommitImpl +import com.intellij.vcs.log.* +import com.intellij.vcs.log.graph.GraphCommit +import com.intellij.vcs.log.graph.GraphCommitImpl +import com.intellij.vcs.log.graph.PermanentGraph +import com.intellij.vcs.log.graph.VisibleGraph +import com.intellij.vcs.log.impl.* +import com.intellij.vcs.log.impl.TestVcsLogProvider.BRANCH_TYPE +import com.intellij.vcs.log.impl.TestVcsLogProvider.DEFAULT_USER +import com.intellij.vcs.log.ui.filter.VcsLogUserFilterImpl +import com.intellij.vcs.log.ui.tables.GraphTableModel +import org.junit.Test +import java.util.ArrayList +import java.util.HashMap +import java.util.HashSet +import kotlin.test.assertEquals +import kotlin.test.assertTrue class VisiblePackBuilderTest { @@ -159,7 +148,17 @@ class VisiblePackBuilderTest { it.value.user, it.value.subject, it.value.user, 1L) Pair(it.key.getId(), metadata) }.toMap() - val builder = VisiblePackBuilder(providers, hashMap, detailsCache, CommitDetailsGetter(hashMap, providers, TRIVIAL_DISPOSABLE)) + + val commitDetailsGetter = object : DataGetter { + override fun getCommitData(row: Int, tableModel: GraphTableModel): VcsFullCommitDetails? { + return null; + } + + override fun getCommitDataIfAvailable(hash: Int): VcsFullCommitDetails? { + return null; + } + } + val builder = VisiblePackBuilder(providers, hashMap, detailsCache, commitDetailsGetter) return builder.build(dataPack, PermanentGraph.SortType.Normal, filters, CommitCountStage.INITIAL).first } @@ -237,11 +236,5 @@ class VisiblePackBuilderTest { override fun findHashByString(string: String) = throw UnsupportedOperationException() } - - object TRIVIAL_DISPOSABLE : Disposable { - override fun dispose() { - throw UnsupportedOperationException() - } - } }