diff: remove LineFragment caching logic
authorAleksey Pivovarov <AMPivovarov@gmail.com>
Thu, 6 Aug 2015 15:59:36 +0000 (18:59 +0300)
committerAleksey Pivovarov <AMPivovarov@gmail.com>
Thu, 6 Aug 2015 15:59:58 +0000 (18:59 +0300)
* its benefits are not obvious
* we can't rely on modificationStamp: document can change, while stamp is left untouched
* it is possible cause of EA-71484 and EA-71274

platform/diff-impl/src/com/intellij/diff/tools/fragmented/UnifiedDiffViewer.java
platform/diff-impl/src/com/intellij/diff/tools/simple/SimpleDiffViewer.java
platform/diff-impl/src/com/intellij/diff/tools/util/LineFragmentCache.java [deleted file]
platform/diff-impl/src/com/intellij/diff/util/DiffUserDataKeysEx.java
platform/diff-impl/src/com/intellij/diff/util/DiffUtil.java

index 76bd143bf666ea315b5888720cf965291054e9ff..b2770697660b68b8d3e74f66f76fe121a970d7c8 100644 (file)
@@ -17,7 +17,6 @@ package com.intellij.diff.tools.fragmented;
 
 import com.intellij.diff.DiffContext;
 import com.intellij.diff.actions.BufferedLineIterator;
-import com.intellij.diff.actions.DocumentFragmentContent;
 import com.intellij.diff.actions.NavigationContextChecker;
 import com.intellij.diff.actions.impl.OpenInEditorWithMouseAction;
 import com.intellij.diff.actions.impl.SetEditorSettingsAction;
@@ -31,7 +30,6 @@ import com.intellij.diff.tools.util.base.*;
 import com.intellij.diff.tools.util.side.TwosideTextDiffViewer;
 import com.intellij.diff.util.*;
 import com.intellij.diff.util.DiffUserDataKeysEx.ScrollToPolicy;
-import com.intellij.diff.util.DiffUtil.DocumentData;
 import com.intellij.openapi.Disposable;
 import com.intellij.openapi.actionSystem.AnAction;
 import com.intellij.openapi.actionSystem.CommonDataKeys;
@@ -238,16 +236,15 @@ public class UnifiedDiffViewer extends ListenerDiffViewerBase {
       final Document document1 = getContent1().getDocument();
       final Document document2 = getContent2().getDocument();
 
-      final DocumentData documentData = ApplicationManager.getApplication().runReadAction(new Computable<DocumentData>() {
+      final CharSequence[] texts = ApplicationManager.getApplication().runReadAction(new Computable<CharSequence[]>() {
         @Override
-        public DocumentData compute() {
-          return new DocumentData(document1.getImmutableCharSequence(), document2.getImmutableCharSequence(),
-                                  document1.getModificationStamp(), document2.getModificationStamp());
+        public CharSequence[] compute() {
+          return new CharSequence[]{document1.getImmutableCharSequence(), document2.getImmutableCharSequence()};
         }
       });
 
       final boolean innerFragments = getDiffConfig().innerFragments;
-      final List<LineFragment> fragments = DiffUtil.compareWithCache(myRequest, documentData, getDiffConfig(), indicator);
+      final List<LineFragment> fragments = DiffUtil.compare(texts[0], texts[1], getDiffConfig(), indicator);
 
       final DocumentContent content1 = getContent1();
       final DocumentContent content2 = getContent2();
@@ -263,7 +260,7 @@ public class UnifiedDiffViewer extends ListenerDiffViewerBase {
           indicator.checkCanceled();
 
           EditorHighlighter highlighter = buildHighlighter(myProject, content1, content2,
-                                                           documentData.getText1(), documentData.getText2(), builder.getRanges(),
+                                                           texts[0], texts[1], builder.getRanges(),
                                                            builder.getText().length());
 
           UnifiedEditorRangeHighlighter rangeHighlighter = new UnifiedEditorRangeHighlighter(myProject, document1, document2,
index e5b41d93cff0fc24a49864a26999ebc373d88da0..4781f04737c1bad345105ab370f5bec719dfd153 100644 (file)
@@ -28,7 +28,6 @@ import com.intellij.diff.tools.util.base.TextDiffViewerUtil;
 import com.intellij.diff.tools.util.side.TwosideTextDiffViewer;
 import com.intellij.diff.util.*;
 import com.intellij.diff.util.DiffUserDataKeysEx.ScrollToPolicy;
-import com.intellij.diff.util.DiffUtil.DocumentData;
 import com.intellij.icons.AllIcons;
 import com.intellij.openapi.Disposable;
 import com.intellij.openapi.actionSystem.AnAction;
@@ -188,17 +187,16 @@ public class SimpleDiffViewer extends TwosideTextDiffViewer {
       final Document document1 = getContent1().getDocument();
       final Document document2 = getContent2().getDocument();
 
-      DocumentData data = ApplicationManager.getApplication().runReadAction(new Computable<DocumentData>() {
+      CharSequence[] texts = ApplicationManager.getApplication().runReadAction(new Computable<CharSequence[]>() {
         @Override
-        public DocumentData compute() {
-          return new DocumentData(document1.getImmutableCharSequence(), document2.getImmutableCharSequence(),
-                                  document1.getModificationStamp(), document2.getModificationStamp());
+        public CharSequence[] compute() {
+          return new CharSequence[]{document1.getImmutableCharSequence(), document2.getImmutableCharSequence()};
         }
       });
 
       List<LineFragment> lineFragments = null;
       if (getHighlightPolicy().isShouldCompare()) {
-        lineFragments = DiffUtil.compareWithCache(myRequest, data, getDiffConfig(), indicator);
+        lineFragments = DiffUtil.compare(texts[0], texts[1], getDiffConfig(), indicator);
       }
 
       boolean isEqualContents = (lineFragments == null || lineFragments.isEmpty()) &&
diff --git a/platform/diff-impl/src/com/intellij/diff/tools/util/LineFragmentCache.java b/platform/diff-impl/src/com/intellij/diff/tools/util/LineFragmentCache.java
deleted file mode 100644 (file)
index f3eadad..0000000
+++ /dev/null
@@ -1,81 +0,0 @@
-/*
- * 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.diff.tools.util;
-
-import com.intellij.diff.comparison.ComparisonPolicy;
-import com.intellij.diff.fragments.LineFragment;
-import org.jetbrains.annotations.NotNull;
-import org.jetbrains.annotations.Nullable;
-
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-public class LineFragmentCache {
-  private final long myModificationStamp1;
-  private final long myModificationStamp2;
-
-  @NotNull private final Map<ComparisonPolicy, PolicyData> myFragments;
-
-  public LineFragmentCache(@NotNull LineFragmentCache cache) {
-    myModificationStamp1 = cache.myModificationStamp1;
-    myModificationStamp2 = cache.myModificationStamp2;
-
-    myFragments = new HashMap<ComparisonPolicy, PolicyData>(3);
-    for (Map.Entry<ComparisonPolicy, PolicyData> entry : cache.myFragments.entrySet()) {
-      myFragments.put(entry.getKey(), entry.getValue());
-    }
-  }
-
-  public LineFragmentCache(long modificationStamp1,
-                           long modificationStamp2) {
-    myModificationStamp1 = modificationStamp1;
-    myModificationStamp2 = modificationStamp2;
-    myFragments = new HashMap<ComparisonPolicy, PolicyData>(3);
-  }
-
-  public boolean checkStamps(long stamp1, long stamp2) {
-    return myModificationStamp1 == stamp1 && myModificationStamp2 == stamp2;
-  }
-
-  @Nullable
-  public PolicyData getData(@NotNull ComparisonPolicy policy) {
-    return myFragments.get(policy);
-  }
-
-  public void putData(@NotNull ComparisonPolicy policy, @NotNull List<LineFragment> fragments, boolean isInnerFragments) {
-    myFragments.put(policy, new PolicyData(fragments, isInnerFragments));
-  }
-
-  public static class PolicyData {
-    @NotNull private final List<LineFragment> myFragments;
-    private final boolean myInnerFragments;
-
-    public PolicyData(@NotNull List<LineFragment> fragments, boolean innerFragments) {
-      myFragments = fragments;
-      myInnerFragments = innerFragments;
-    }
-
-    @NotNull
-    public List<LineFragment> getFragments() {
-      return myFragments;
-    }
-
-    public boolean isInnerFragments() {
-      return myInnerFragments;
-    }
-  }
-}
index 703ff32061b9c8a146a90d5c8bdafe3496f87017..a2c90ce8e5b234df68f709a74982d78b08c353fc 100644 (file)
@@ -15,7 +15,6 @@
  */
 package com.intellij.diff.util;
 
-import com.intellij.diff.tools.util.LineFragmentCache;
 import com.intellij.openapi.diff.DiffNavigationContext;
 import com.intellij.openapi.editor.LogicalPosition;
 import com.intellij.openapi.util.Key;
@@ -45,7 +44,6 @@ public interface DiffUserDataKeysEx extends DiffUserDataKeys {
   Key<LogicalPosition[]> EDITORS_CARET_POSITION = Key.create("Diff.EditorsCaretPosition");
 
   Key<DiffNavigationContext> NAVIGATION_CONTEXT = Key.create("Diff.NavigationContext");
-  Key<LineFragmentCache> LINE_FRAGMENT_CACHE = Key.create("Diff.LineFragmentCache");
 
   //
   // DiffContext
index 34b125b8c6bc5830e5bb79a02929860ad3048fbd..23a30d141232a5eb0a0be41de884ca3200ae3f02 100644 (file)
@@ -30,8 +30,6 @@ import com.intellij.diff.fragments.DiffFragment;
 import com.intellij.diff.fragments.LineFragment;
 import com.intellij.diff.requests.ContentDiffRequest;
 import com.intellij.diff.requests.DiffRequest;
-import com.intellij.diff.tools.util.LineFragmentCache;
-import com.intellij.diff.tools.util.LineFragmentCache.PolicyData;
 import com.intellij.diff.tools.util.base.HighlightPolicy;
 import com.intellij.diff.tools.util.base.IgnorePolicy;
 import com.intellij.openapi.actionSystem.AnAction;
@@ -464,86 +462,23 @@ public class DiffUtil {
   //
 
   @NotNull
-  public static List<LineFragment> compareWithCache(@NotNull DiffRequest request,
-                                                    @NotNull DocumentData data,
-                                                    @NotNull DiffConfig config,
-                                                    @NotNull ProgressIndicator indicator) {
-    return compareWithCache(request, data.getText1(), data.getText2(), data.getStamp1(), data.getStamp2(), config, indicator);
-  }
-
-  @NotNull
-  public static List<LineFragment> compareWithCache(@NotNull DiffRequest request,
-                                                    @NotNull CharSequence text1,
-                                                    @NotNull CharSequence text2,
-                                                    long stamp1,
-                                                    long stamp2,
-                                                    @NotNull DiffConfig config,
-                                                    @NotNull ProgressIndicator indicator) {
-    List<LineFragment> fragments = doCompareWithCache(request, text1, text2, stamp1, stamp2, config, indicator);
-
+  public static List<LineFragment> compare(@NotNull CharSequence text1,
+                                           @NotNull CharSequence text2,
+                                           @NotNull DiffConfig config,
+                                           @NotNull ProgressIndicator indicator) {
     indicator.checkCanceled();
-    return ComparisonManager.getInstance().processBlocks(fragments, text1, text2,
-                                                         config.policy, config.squashFragments, config.trimFragments);
-  }
 
-  @NotNull
-  private static List<LineFragment> doCompareWithCache(@NotNull DiffRequest request,
-                                                       @NotNull CharSequence text1,
-                                                       @NotNull CharSequence text2,
-                                                       long stamp1,
-                                                       long stamp2,
-                                                       @NotNull DiffConfig config,
-                                                       @NotNull ProgressIndicator indicator) {
-    indicator.checkCanceled();
-    PolicyData cachedData = getFromCache(request, config, stamp1, stamp2);
-
-    List<LineFragment> newFragments;
-    if (cachedData != null) {
-      if (cachedData.getFragments().isEmpty()) return cachedData.getFragments();
-      if (!config.innerFragments) return cachedData.getFragments();
-      if (cachedData.isInnerFragments()) return cachedData.getFragments();
-      newFragments = ComparisonManager.getInstance().compareLinesInner(text1, text2, cachedData.getFragments(), config.policy, indicator);
+    List<LineFragment> fragments;
+    if (config.innerFragments) {
+      fragments = ComparisonManager.getInstance().compareLinesInner(text1, text2, config.policy, indicator);
     }
     else {
-      if (config.innerFragments) {
-        newFragments = ComparisonManager.getInstance().compareLinesInner(text1, text2, config.policy, indicator);
-      }
-      else {
-        newFragments = ComparisonManager.getInstance().compareLines(text1, text2, config.policy, indicator);
-      }
+      fragments = ComparisonManager.getInstance().compareLines(text1, text2, config.policy, indicator);
     }
 
     indicator.checkCanceled();
-    putToCache(request, config, stamp1, stamp2, newFragments, config.innerFragments);
-    return newFragments;
-  }
-
-  @Nullable
-  public static PolicyData getFromCache(@NotNull DiffRequest request, @NotNull DiffConfig config, long stamp1, long stamp2) {
-    LineFragmentCache cache = request.getUserData(DiffUserDataKeysEx.LINE_FRAGMENT_CACHE);
-    if (cache != null && cache.checkStamps(stamp1, stamp2)) {
-      return cache.getData(config.policy);
-    }
-    return null;
-  }
-
-  public static void putToCache(@NotNull DiffRequest request, @NotNull DiffConfig config, long stamp1, long stamp2,
-                                @NotNull List<LineFragment> fragments, boolean isInnerFragments) {
-    // We can't rely on monotonicity on modificationStamps, so we can't check if we actually compared freshest versions of documents
-    // Possible data races also could make cache outdated.
-    // But these cases shouldn't be often and won't break anything.
-
-    LineFragmentCache oldCache = request.getUserData(DiffUserDataKeysEx.LINE_FRAGMENT_CACHE);
-    LineFragmentCache cache;
-    if (oldCache == null || !oldCache.checkStamps(stamp1, stamp2)) {
-      cache = new LineFragmentCache(stamp1, stamp2);
-    }
-    else {
-      cache = new LineFragmentCache(oldCache);
-    }
-
-    cache.putData(config.policy, fragments, isInnerFragments);
-    request.putUserData(DiffUserDataKeysEx.LINE_FRAGMENT_CACHE, cache);
+    return ComparisonManager.getInstance().processBlocks(fragments, text1, text2,
+                                                         config.policy, config.squashFragments, config.trimFragments);
   }
 
   //
@@ -983,38 +918,6 @@ public class DiffUtil {
   // Helpers
   //
 
-  public static class DocumentData {
-    @NotNull private final CharSequence myText1;
-    @NotNull private final CharSequence myText2;
-    private final long myStamp1;
-    private final long myStamp2;
-
-    public DocumentData(@NotNull CharSequence text1, @NotNull CharSequence text2, long stamp1, long stamp2) {
-      myText1 = text1;
-      myText2 = text2;
-      myStamp1 = stamp1;
-      myStamp2 = stamp2;
-    }
-
-    @NotNull
-    public CharSequence getText1() {
-      return myText1;
-    }
-
-    @NotNull
-    public CharSequence getText2() {
-      return myText2;
-    }
-
-    public long getStamp1() {
-      return myStamp1;
-    }
-
-    public long getStamp2() {
-      return myStamp2;
-    }
-  }
-
   public static class DiffConfig {
     @NotNull public final ComparisonPolicy policy;
     public final boolean innerFragments;