From b13fb219a74c369df59e7baa4d255a78fd8e2ec4 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Tue, 7 Jul 2026 09:34:11 +0200 Subject: [PATCH] Avoid redundant content reads when opening a compare editor Opening a compare editor read each side's contents up to 15 times: the content and structure switching panes looked the viewer descriptors up twice per pane, and every lookup re-ran content-type sniffing and the text heuristic against a fresh stream. The panes now cache the descriptor array computed while creating the viewer, and the sniffing results are memoized per element for the duration of one open, keyed by element identity and reset whenever a different input is examined. Each side is now read at most three times: once for content-type detection, once for the text heuristic, and once for the document shown in the viewer. Viewer selection is unchanged. Contributes to https://github.com/eclipse-platform/eclipse.platform/issues/2795 --- .../CompareContentViewerSwitchingPane.java | 25 ++++++++--- .../CompareStructureViewerSwitchingPane.java | 16 ++++++-- .../compare/internal/CompareUIPlugin.java | 41 +++++++++++++++++++ .../tests/CompareOpenEfficiencyTest.java | 8 ++-- 4 files changed, 77 insertions(+), 13 deletions(-) diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareContentViewerSwitchingPane.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareContentViewerSwitchingPane.java index 6df3ac68766..2a7afd10b02 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareContentViewerSwitchingPane.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareContentViewerSwitchingPane.java @@ -60,6 +60,9 @@ public class CompareContentViewerSwitchingPane extends CompareViewerSwitchingPan private ViewerDescriptor fSelectedViewerDescriptor; + /** Descriptors computed by getViewer, reused by setInput to avoid a second lookup. */ + private ViewerDescriptor[] fViewerDescriptors; + private ToolBar toolBar; private CLabel labelOptimized; private Link recomputeLink; @@ -79,9 +82,10 @@ private CompareConfiguration getCompareConfiguration() { @Override protected Viewer getViewer(Viewer oldViewer, Object input) { if (fSelectedViewerDescriptor != null) { - ViewerDescriptor[] array = CompareUIPlugin.getDefault().findContentViewerDescriptor( + fViewerDescriptors = CompareUIPlugin.getDefault().findContentViewerDescriptor( oldViewer, input, getCompareConfiguration()); - List list = array != null ? Arrays.asList(array) : Collections.emptyList(); + List list = fViewerDescriptors != null ? Arrays.asList(fViewerDescriptors) + : Collections.emptyList(); if (list.contains(fSelectedViewerDescriptor)) { // use selected viewer only when appropriate for the new input fCompareEditorInput @@ -94,12 +98,19 @@ protected Viewer getViewer(Viewer oldViewer, Object input) { fSelectedViewerDescriptor = null; } if (input instanceof ICompareInput) { - fCompareEditorInput.setContentViewerDescriptor(null); + fViewerDescriptors = CompareUIPlugin.getDefault().findContentViewerDescriptor( + oldViewer, input, getCompareConfiguration()); + // Feeding the default descriptor avoids a second lookup inside findContentViewer. + ViewerDescriptor defaultDescriptor = fViewerDescriptors != null && fViewerDescriptors.length > 0 + ? fViewerDescriptors[0] + : null; + fCompareEditorInput.setContentViewerDescriptor(defaultDescriptor); Viewer viewer = fCompareEditorInput.findContentViewer(oldViewer, (ICompareInput) input, this); fCompareEditorInput.setContentViewerDescriptor(fSelectedViewerDescriptor); return viewer; } + fViewerDescriptors = null; return null; } @@ -203,8 +214,12 @@ public void setInput(Object input) { if (getViewer() == null || !Utilities.okToUse(getViewer().getControl())) { return; } - ViewerDescriptor[] vd = CompareUIPlugin.getDefault() - .findContentViewerDescriptor(getViewer(), getInput(), getCompareConfiguration()); + // Reuse the descriptors computed by getViewer; recompute only if the cache is empty. + ViewerDescriptor[] vd = fViewerDescriptors; + if (vd == null) { + vd = CompareUIPlugin.getDefault() + .findContentViewerDescriptor(getViewer(), getInput(), getCompareConfiguration()); + } boolean toolbarVisible = vd != null && vd.length > 1; toolBar.setVisible(toolbarVisible); ((RowData) toolBar.getLayoutData()).exclude = !toolbarVisible; diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareStructureViewerSwitchingPane.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareStructureViewerSwitchingPane.java index 575a100b9c6..8e8f35cfdac 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareStructureViewerSwitchingPane.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareStructureViewerSwitchingPane.java @@ -52,6 +52,9 @@ public class CompareStructureViewerSwitchingPane extends private ViewerDescriptor fSelectedViewerDescriptor; + /** Descriptors computed by getViewer, reused by setInput to avoid a second lookup. */ + private ViewerDescriptor[] fViewerDescriptors; + private ToolBar toolBar; public CompareStructureViewerSwitchingPane(Composite parent, int style, @@ -68,9 +71,10 @@ private CompareConfiguration getCompareConfiguration() { protected Viewer getViewer(Viewer oldViewer, Object input) { if (input instanceof ICompareInput) { if (fSelectedViewerDescriptor != null) { - ViewerDescriptor[] array = CompareUIPlugin.getDefault().findStructureViewerDescriptor( + fViewerDescriptors = CompareUIPlugin.getDefault().findStructureViewerDescriptor( oldViewer, (ICompareInput)input, getCompareConfiguration()); - List list = array != null ? Arrays.asList(array) : Collections.emptyList(); + List list = fViewerDescriptors != null ? Arrays.asList(fViewerDescriptors) + : Collections.emptyList(); if (list.contains(fSelectedViewerDescriptor)) { // use selected viewer only when appropriate for the new input fCompareEditorInput @@ -83,12 +87,15 @@ protected Viewer getViewer(Viewer oldViewer, Object input) { fSelectedViewerDescriptor = null; } + // The default path computes no descriptors; invalidate so setInput recomputes. + fViewerDescriptors = null; fCompareEditorInput.setStructureViewerDescriptor(null); Viewer viewer = fCompareEditorInput.findStructureViewer(oldViewer, (ICompareInput) input, this); fCompareEditorInput.setStructureViewerDescriptor(fSelectedViewerDescriptor); return viewer; } + fViewerDescriptors = null; return null; } @@ -141,8 +148,9 @@ public void setInput(Object input) { if (getViewer() == null || !Utilities.okToUse(getViewer().getControl())) { return; } - ViewerDescriptor[] vd = null; - if (getInput() instanceof ICompareInput) { + // Reuse the descriptors computed by getViewer; recompute only if the cache is empty. + ViewerDescriptor[] vd = fViewerDescriptors; + if (vd == null && getInput() instanceof ICompareInput) { vd = CompareUIPlugin.getDefault().findStructureViewerDescriptor( getViewer(), (ICompareInput) getInput(), getCompareConfiguration()); diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareUIPlugin.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareUIPlugin.java index f380ca3d85f..7b134dfea2e 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareUIPlugin.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/CompareUIPlugin.java @@ -30,6 +30,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Hashtable; +import java.util.IdentityHashMap; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -261,6 +262,14 @@ Collection getAll() { // content type private static final IContentTypeManager fgContentTypeManager= Platform.getContentTypeManager(); + /** + * Per-open memoization of content-type sniffing, keyed by element identity and + * reset when a different input is examined. Only accessed on the UI thread. + */ + private static Object fSniffInput; + private static final Map fContentTypeSniffCache= new IdentityHashMap<>(); + private static final Map fGuessTypeSniffCache= new IdentityHashMap<>(); + public static final int NO_DIFFERENCE = 10000; /** @@ -1060,6 +1069,7 @@ public IStreamMerger createStreamMerger(IContentType type) { public ViewerDescriptor[] findStructureViewerDescriptor(Viewer oldViewer, ICompareInput input, CompareConfiguration configuration) { + beginContentTypeSniffing(input); // we don't show the structure of additions or deletions if ((input == null) || input == null || input.getLeft() == null || input.getRight() == null) { return null; @@ -1245,6 +1255,7 @@ private Collection getContentTypes(Object in) { } public ViewerDescriptor[] findContentViewerDescriptor(Viewer oldViewer, Object in, CompareConfiguration cc) { + beginContentTypeSniffing(in); LinkedHashSet result = new LinkedHashSet<>(); if (in instanceof IStreamContentAccessor) { String type= ITypedElement.TEXT_TYPE; @@ -1469,10 +1480,28 @@ private static String[] getTypes(ICompareInput input) { return tmp.toArray(new String[tmp.size()]); } + /** Resets the sniffing caches when a different input is examined. */ + private static void beginContentTypeSniffing(Object input) { + if (input != fSniffInput) { + fContentTypeSniffCache.clear(); + fGuessTypeSniffCache.clear(); + fSniffInput= input; + } + } + private static IContentType getContentType(ITypedElement element) { if (element == null) { return null; } + if (fContentTypeSniffCache.containsKey(element)) { + return fContentTypeSniffCache.get(element); + } + IContentType ct= computeContentType(element); + fContentTypeSniffCache.put(element, ct); + return ct; + } + + private static IContentType computeContentType(ITypedElement element) { String name= element.getName(); IContentType ct= null; if (element instanceof IResourceProvider) { @@ -1603,6 +1632,18 @@ private static IContentType[] toFullPath(IContentType ct) { * Returns null if the input isn't an IStreamContentAccessor. */ private static String guessType(ITypedElement input) { + if (input == null) { + return null; + } + if (fGuessTypeSniffCache.containsKey(input)) { + return fGuessTypeSniffCache.get(input); + } + String guessed= computeGuessType(input); + fGuessTypeSniffCache.put(input, guessed); + return guessed; + } + + private static String computeGuessType(ITypedElement input) { if (input instanceof IStreamContentAccessor sca) { InputStream is= null; try { diff --git a/team/tests/org.eclipse.compare.tests/src/org/eclipse/compare/tests/CompareOpenEfficiencyTest.java b/team/tests/org.eclipse.compare.tests/src/org/eclipse/compare/tests/CompareOpenEfficiencyTest.java index 4e004283670..39658f927c9 100644 --- a/team/tests/org.eclipse.compare.tests/src/org/eclipse/compare/tests/CompareOpenEfficiencyTest.java +++ b/team/tests/org.eclipse.compare.tests/src/org/eclipse/compare/tests/CompareOpenEfficiencyTest.java @@ -55,11 +55,11 @@ public class CompareOpenEfficiencyTest { /** * Upper bound for the {@code getContents()} calls per side during a single - * compare editor open, caused by repeated content-type sniffing and viewer - * descriptor lookups. The exact count is platform dependent (observed: 15 on - * Linux and Windows, 9 on macOS), so only the worst case is asserted. + * compare editor open: one read for content-type detection, one for the text + * heuristic, and one for the document shown in the viewer. Asserted as a bound + * because the exact count can vary by platform. */ - private static final int MAX_GET_CONTENTS_PER_SIDE = 15; + private static final int MAX_GET_CONTENTS_PER_SIDE = 3; private static final long TIMEOUT_MILLIS = 30_000;