From c1ff1d19f59a4f757d1131abbde45e4e266f5a7a Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 30 Jul 2026 19:12:28 +0200 Subject: [PATCH] Read the compared contents once, in the background Opening a compare editor read each side three times: once for content type detection, once for the text heuristic and once for the document shown in the viewer, all three on the UI thread and each one paying whatever the provider charges for opening a stream. For an EGit revision that means inflating the blob and running the smudge filters three times over. The background job that prepares the input now reads the contents once and the sniffing and the merge viewer are served from that. Elements backed by a shared document are skipped, since those come from the file buffer and never read a stream anyway. Only contents that look like text are kept, and only up to 8 MB of them. The binary viewer stops at the first differing byte, so reading a binary in full ahead of it would be slower than not prefetching at all, and the text merge viewer never sees it. Deciding that costs an 8 KB probe. What is kept is held weakly and dropped as soon as it became a document, so it goes away with the editor that prepared it even when no viewer asks for it. The read checks the monitor, so cancelling stays responsive on exactly the slow providers this targets. CompareOpenEfficiencyTest's bound drops from 3 reads per side to 1, and a new test pins the binary side to a bounded number of bytes. Contributes to https://github.com/eclipse-platform/eclipse.platform/issues/2795 --- .../contentmergeviewer/TextMergeViewer.java | 6 +- .../compare/internal/CompareUIPlugin.java | 161 +++++++++++++++++- .../eclipse/compare/internal/Utilities.java | 19 ++- .../tests/CompareOpenEfficiencyTest.java | 87 +++++++++- 4 files changed, 262 insertions(+), 11 deletions(-) diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java index 06d16a0519a..e9ecc883494 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java @@ -893,7 +893,11 @@ private IDocument createDocument() { try { String encoding = internalGetEncoding(); - s = Utilities.readString(sca, encoding); + // The background job may have read the contents already; decoding is + // all that is left then. + byte[] prefetched = CompareUIPlugin.takePrefetchedContents(fElement); + s = prefetched != null ? Utilities.readString(prefetched, encoding) + : Utilities.readString(sca, encoding); } catch (CoreException ex) { this.fViewer.setError(fLeg, ex.getMessage()); } 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 9f1f2605924..85883ec0ada 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 @@ -17,6 +17,8 @@ package org.eclipse.compare.internal; import java.io.BufferedInputStream; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.lang.ref.Reference; @@ -285,6 +287,30 @@ private static final class SniffedElement { } } + /** + * Contents read ahead while preparing an input. They are written in the + * background job and read on the UI thread. An entry is dropped as soon as it + * became a document, and the element is held weakly so that entries nobody asked + * for go away with the editor that prepared them. + */ + private static final List fPrefetchedContents= new ArrayList<>(3); + + /** Contents larger than this are read by whoever needs them instead. */ + private static final int PREFETCH_LIMIT= 8 * 1024 * 1024; + + /** Enough for the content type describers and the text heuristic. */ + private static final int PREFETCH_PROBE_SIZE= 8 * 1024; + + private static final class PrefetchedElement { + final Reference element; + final byte[] contents; + + PrefetchedElement(ITypedElement element, byte[] contents) { + this.element= new WeakReference<>(element); + this.contents= contents; + } + } + public static final int NO_DIFFERENCE = 10000; /** @@ -904,6 +930,9 @@ public IStatus prepareInput(CompareEditorInput input, IProgressMonitor monitor) if (input.getCompareResult() == null) { return new Status(IStatus.ERROR, CompareUIPlugin.PLUGIN_ID, NO_DIFFERENCE, Utilities.getString("CompareUIPlugin.noDifferences"), null); //$NON-NLS-1$ } + // Still off the UI thread here, so this is the place to read the contents + // that the viewer would otherwise read while the user waits. + prefetchContents(input.getCompareResult(), monitor); return Status.OK_STATUS; } catch (InterruptedException e) { throw new OperationCanceledException(); @@ -1569,6 +1598,134 @@ private static IContentType getContentType(ITypedElement element) { return sniffed.contentType; } + /** + * Reads the contents of the given input's elements once, so that the sniffing + * and the merge viewer do not have to open the streams again on the UI thread. + * Elements that come with a shared document are left alone: those are served + * from the file buffer and never read a stream. + */ + public static void prefetchContents(Object compareResult, IProgressMonitor monitor) { + if (!(compareResult instanceof ICompareInput input)) { + return; + } + prefetchContents(input.getAncestor(), monitor); + prefetchContents(input.getLeft(), monitor); + prefetchContents(input.getRight(), monitor); + } + + private static void prefetchContents(ITypedElement element, IProgressMonitor monitor) { + if (!(element instanceof IStreamContentAccessor accessor) || hasSharedDocument(element)) { + return; + } + try (InputStream stream= accessor.getContents()) { + if (stream == null) { + return; + } + // Binary contents are neither shown by the text merge viewer nor read in + // full by the binary one, so nothing would use them. + byte[] probe= read(stream, PREFETCH_PROBE_SIZE, monitor); + if (probe == null || !looksLikeText(probe)) { + return; + } + byte[] rest= read(stream, PREFETCH_LIMIT - probe.length + 1, monitor); + if (rest == null || probe.length + rest.length > PREFETCH_LIMIT) { + return; // too large to be worth holding on to + } + byte[] contents= new byte[probe.length + rest.length]; + System.arraycopy(probe, 0, contents, 0, probe.length); + System.arraycopy(rest, 0, contents, probe.length, rest.length); + putPrefetchedContents(element, contents); + } catch (CoreException | IOException e) { + // not fatal: whoever needs the contents reads the stream itself + } + } + + /** Reads up to the given number of bytes, or null when cancelled. */ + private static byte[] read(InputStream stream, int limit, IProgressMonitor monitor) throws IOException { + ByteArrayOutputStream buffer= new ByteArrayOutputStream(Math.min(limit, 8192)); + byte[] chunk= new byte[8192]; + while (buffer.size() < limit) { + if (monitor != null && monitor.isCanceled()) { + return null; + } + int read= stream.read(chunk, 0, Math.min(chunk.length, limit - buffer.size())); + if (read == -1) { + break; + } + buffer.write(chunk, 0, read); + } + return buffer.toByteArray(); + } + + /** The heuristic of {@link #computeGuessType(ITypedElement)}, on bytes at hand. */ + private static boolean looksLikeText(byte[] contents) { + int lineLength= 0; + int lines= 0; + for (int i= 0; i < contents.length && lines < 10; i++) { + byte c= contents[i]; + if (c == '\n' || c == '\r') { + lineLength= 0; + lines++; + } else if (++lineLength > 1000) { + return false; + } + } + return true; + } + + private static boolean hasSharedDocument(ITypedElement element) { + ISharedDocumentAdapter adapter= SharedDocumentAdapterWrapper.getAdapter(element); + return adapter != null && adapter.getDocumentKey(element) != null; + } + + private static void putPrefetchedContents(ITypedElement element, byte[] contents) { + synchronized (fPrefetchedContents) { + fPrefetchedContents.removeIf(prefetched -> { + ITypedElement held= prefetched.element.get(); + return held == null || held == element; + }); + fPrefetchedContents.add(new PrefetchedElement(element, contents)); + } + } + + /** + * Returns the prefetched contents of the given element and forgets them, or + * null if nothing was read ahead for it. + */ + public static byte[] takePrefetchedContents(Object element) { + synchronized (fPrefetchedContents) { + for (Iterator it= fPrefetchedContents.iterator(); it.hasNext();) { + PrefetchedElement prefetched= it.next(); + ITypedElement held= prefetched.element.get(); + if (held == null) { + it.remove(); + } else if (held == element) { + it.remove(); + return prefetched.contents; + } + } + } + return null; + } + + private static byte[] prefetchedContents(ITypedElement element) { + synchronized (fPrefetchedContents) { + for (PrefetchedElement prefetched : fPrefetchedContents) { + if (prefetched.element.get() == element) { + return prefetched.contents; + } + } + } + return null; + } + + /** The contents of the given element, read ahead if that already happened. */ + private static InputStream contentsOf(ITypedElement element, IStreamContentAccessor accessor) + throws CoreException { + byte[] prefetched= prefetchedContents(element); + return prefetched != null ? new ByteArrayInputStream(prefetched) : accessor.getContents(); + } + private static IContentType computeContentType(ITypedElement element) { String name= element.getName(); IContentType ct= null; @@ -1585,7 +1742,7 @@ private static IContentType computeContentType(ITypedElement element) { } if (element instanceof IStreamContentAccessor isa) { try { - InputStream is= isa.getContents(); + InputStream is= contentsOf(element, isa); if (is != null) { try (InputStream bis = new BufferedInputStream(is)) { ct= fgContentTypeManager.findContentTypeFor(is, name); @@ -1718,7 +1875,7 @@ private static String computeGuessType(ITypedElement input) { if (input instanceof IStreamContentAccessor sca) { InputStream is= null; try { - is= sca.getContents(); + is= contentsOf(input, sca); if (is == null) { return null; } diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/Utilities.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/Utilities.java index ba1dc5c5897..9e7ec60d762 100644 --- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/Utilities.java +++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/Utilities.java @@ -15,6 +15,7 @@ package org.eclipse.compare.internal; import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -582,14 +583,28 @@ private static IStatus addStatus(IStatus status, IStatus entry) { // encoding + /** Opens the contents to decode, once per decoding attempt. */ + private interface ContentsSupplier { + InputStream get() throws CoreException; + } + + /** Decodes contents that have already been read. */ + public static String readString(byte[] contents, String encoding) throws CoreException { + return decode(() -> new ByteArrayInputStream(contents), encoding); + } + public static String readString(IStreamContentAccessor sca, String encoding) throws CoreException { + return decode(sca::getContents, encoding); + } + + private static String decode(ContentsSupplier contents, String encoding) throws CoreException { String s = null; try { try { - s= Utilities.readString(sca.getContents(), encoding); + s= Utilities.readString(contents.get(), encoding); } catch (UnsupportedEncodingException e) { if (!encoding.equals(ResourcesPlugin.getEncoding())) { - s = Utilities.readString(sca.getContents(), ResourcesPlugin.getEncoding()); + s = Utilities.readString(contents.get(), ResourcesPlugin.getEncoding()); } } } catch (IOException e) { 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 c3fa70ae74a..66bba0c8c58 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 @@ -19,9 +19,13 @@ import static org.junit.jupiter.api.Assertions.fail; import java.io.ByteArrayInputStream; +import java.io.FilterInputStream; +import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.BooleanSupplier; import org.eclipse.compare.CompareConfiguration; @@ -55,31 +59,68 @@ public class CompareOpenEfficiencyTest { /** * Upper bound for the {@code getContents()} calls per side on one compare editor - * open: content-type detection, text heuristic, and the document itself. + * open: the background job reads the contents once and everything else is served + * from it. */ - private static final int MAX_GET_CONTENTS_PER_SIDE = 3; + private static final int MAX_GET_CONTENTS_PER_SIDE = 1; + + private static final int BINARY_SIZE = 4 * 1024 * 1024; + + /** + * Only the probe that tells binary from text may be read per side, plus a little + * slack for the content type describers. + */ + private static final long MAX_BINARY_BYTES_PER_SIDE = 64 * 1024; private static final long TIMEOUT_MILLIS = 30_000; private boolean originalUnifiedDiff; - /** A text element that counts every {@link #getContents()} call. */ + /** + * An element that counts every {@link #getContents()} call and every byte + * consumed from the streams it hands out. + */ private static final class CountingElement implements ITypedElement, IEncodedStreamContentAccessor { private final String name; + private final String type; private final byte[] bytes; private final AtomicInteger contentReads = new AtomicInteger(); + private final AtomicLong bytesRead = new AtomicLong(); CountingElement(String name, String content) { + this(name, TEXT_TYPE, content.getBytes(StandardCharsets.UTF_8)); + } + + CountingElement(String name, String type, byte[] content) { this.name = name; - this.bytes = content.getBytes(StandardCharsets.UTF_8); + this.type = type; + this.bytes = content; } @Override public InputStream getContents() { contentReads.incrementAndGet(); - return new ByteArrayInputStream(bytes); + return new FilterInputStream(new ByteArrayInputStream(bytes)) { + @Override + public int read() throws IOException { + int b = super.read(); + if (b != -1) { + bytesRead.incrementAndGet(); + } + return b; + } + + @Override + public int read(byte[] b, int off, int len) throws IOException { + int n = super.read(b, off, len); + if (n > 0) { + bytesRead.addAndGet(n); + } + return n; + } + }; } @Override @@ -94,7 +135,7 @@ public String getName() { @Override public String getType() { - return TEXT_TYPE; + return type; } @Override @@ -102,6 +143,10 @@ public Image getImage() { return null; } + long bytes() { + return bytesRead.get(); + } + int reads() { return contentReads.get(); } @@ -188,6 +233,36 @@ public void testGetContentsCallCountPerSide() throws Exception { assertReadsWithinBound("right", rightElement.reads()); //$NON-NLS-1$ } + /** + * Binary contents must not be read ahead: the binary viewer stops at the first + * difference, so buffering the whole thing would be both slower and a lot of + * memory held for nothing. + */ + @Test + public void testBinaryContentsAreNotReadAhead() throws Exception { + store().setValue(ComparePreferencePage.UNIFIED_DIFF, false); + byte[] leftBytes = new byte[BINARY_SIZE]; + Arrays.fill(leftBytes, (byte) 1); + byte[] rightBytes = leftBytes.clone(); + rightBytes[0] = 2; // differs immediately, so the viewer reads one byte + CountingElement leftElement = new CountingElement("left.bin", ITypedElement.UNKNOWN_TYPE, leftBytes); //$NON-NLS-1$ + CountingElement rightElement = new CountingElement("right.bin", ITypedElement.UNKNOWN_TYPE, rightBytes); //$NON-NLS-1$ + CountingCompareEditorInput input = new CountingCompareEditorInput(true, leftElement, rightElement); + + CompareUI.openCompareEditor(input); + pumpUntil(() -> input.getCompareResult() != null && contentPane(input) != null, + "compare editor did not finish opening"); //$NON-NLS-1$ + + assertBytesWithinBound("left", leftElement.bytes()); //$NON-NLS-1$ + assertBytesWithinBound("right", rightElement.bytes()); //$NON-NLS-1$ + } + + private static void assertBytesWithinBound(String side, long bytes) { + assertTrue(bytes <= MAX_BINARY_BYTES_PER_SIDE, + "read " + bytes + " bytes from the " + side + " side of a " + BINARY_SIZE //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + + " byte binary, exceeding the bound of " + MAX_BINARY_BYTES_PER_SIDE); //$NON-NLS-1$ + } + private static void assertReadsWithinBound(String side, int reads) { assertTrue(reads >= 1, "expected at least one getContents() call on the " + side + " side"); //$NON-NLS-1$ //$NON-NLS-2$ assertTrue(reads <= MAX_GET_CONTENTS_PER_SIDE,