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,