From 7b037a48e60c39d21ff68ff2198d584e68ac5bee Mon Sep 17 00:00:00 2001 From: sebastien-doyon Date: Tue, 26 Sep 2023 15:36:00 +0200 Subject: [PATCH 1/4] [MNG-7899] Various memory usage improvements Merging the getStatus() method to optimize : - Use the main StringBuilder to append string instead of using a separate one - Use the StringBuilder.append() with index to avoid String.substring(), less temporary strings - Reuse the FileSizeFormat object in the while loop avoiding multiple temporary instances creation --- .../ConsoleMavenTransferListener.java | 44 ++++++++++--------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java index a0045f712a7c..c7b132adc5b6 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java +++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java @@ -68,13 +68,34 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer buffer.append("Progress (").append(transfers.size()).append("): "); synchronized (transfers) { + FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); + Iterator> entries = transfers.entrySet().iterator(); while (entries.hasNext()) { Map.Entry entry = entries.next(); long total = entry.getKey().getContentLength(); Long complete = entry.getValue(); - buffer.append(getStatus(entry.getKey().getResourceName(), complete, total)); + + String resourceName = entry.getKey().getResourceName(); + + if (printResourceNames) { + int idx = resourceName.lastIndexOf('/'); + + if (idx < 0) { + buffer.append(resourceName); + } else { + buffer.append(resourceName, idx + 1, resourceName.length()); + } + buffer.append(" ("); + } + + buffer.append(format.formatProgress(complete, total)); + + if (printResourceNames) { + buffer.append(")"); + } + if (entries.hasNext()) { buffer.append(" | "); } @@ -89,25 +110,6 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer out.flush(); } - private String getStatus(String resourceName, long complete, long total) { - FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); - StringBuilder status = new StringBuilder(); - - if (printResourceNames) { - int idx = resourceName.lastIndexOf('/'); - status.append(idx < 0 ? resourceName : resourceName.substring(idx + 1)); - status.append(" ("); - } - - status.append(format.formatProgress(complete, total)); - - if (printResourceNames) { - status.append(")"); - } - - return status.toString(); - } - private void pad(StringBuilder buffer, int spaces) { String block = " "; while (spaces > 0) { @@ -135,7 +137,7 @@ public synchronized void transferFailed(TransferEvent event) { private void overridePreviousTransfer(TransferEvent event) { if (lastLength > 0) { - StringBuilder buffer = new StringBuilder(128); + StringBuilder buffer = new StringBuilder(lastLength + 1); pad(buffer, lastLength); buffer.append('\r'); out.print(buffer); From 730d0b5789ac644ed81264e369c50eb00fe2bbb8 Mon Sep 17 00:00:00 2001 From: sebastien-doyon Date: Thu, 12 Oct 2023 17:50:12 +0200 Subject: [PATCH 2/4] [MNG-7899] Various memory usage improvements - Non-threadsafe FileSizeFormat instance can be make class instance since its formatProgress() method is only called in a synchronized block. - add a test in a multi-threaded context --- .../ConsoleMavenTransferListener.java | 3 +- .../ConsoleMavenTransferListenerTest.java | 128 ++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 maven-embedder/src/test/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListenerTest.java diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java index c7b132adc5b6..be13b815b7c0 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java +++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java @@ -36,6 +36,7 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener { private Map transfers = Collections.synchronizedMap(new LinkedHashMap<>()); + private FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); // use in a synchronized fashion private boolean printResourceNames; private int lastLength; @@ -68,8 +69,6 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer buffer.append("Progress (").append(transfers.size()).append("): "); synchronized (transfers) { - FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); - Iterator> entries = transfers.entrySet().iterator(); while (entries.hasNext()) { diff --git a/maven-embedder/src/test/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListenerTest.java b/maven-embedder/src/test/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListenerTest.java new file mode 100644 index 000000000000..095124052c16 --- /dev/null +++ b/maven-embedder/src/test/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListenerTest.java @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.maven.cli.transfer; + +import java.io.FileNotFoundException; +import java.io.PrintStream; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import org.eclipse.aether.DefaultRepositorySystemSession; +import org.eclipse.aether.transfer.TransferCancelledException; +import org.eclipse.aether.transfer.TransferEvent; +import org.eclipse.aether.transfer.TransferResource; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ConsoleMavenTransferListenerTest { + + private CountDownLatch startLatch; + private CountDownLatch endLatch; + + @Test + void testTransferProgressedWithPrintResourceNames() throws FileNotFoundException, InterruptedException { + int size = 1000; + ExecutorService service = Executors.newFixedThreadPool(size * 2); + startLatch = new CountDownLatch(size); + endLatch = new CountDownLatch(size); + Map output = new ConcurrentHashMap(); + + ConsoleMavenTransferListener listener = new ConsoleMavenTransferListener( + new PrintStream(System.out) { + + @Override + public void print(Object o) { + + String string = o.toString(); + int i = string.length() - 1; + while (i >= 0) { + char c = string.charAt(i); + if (c == '\n' || c == '\r' || c == ' ') i--; + else break; + } + + string = string.substring(0, i + 1).trim(); + output.put(string, string); + System.out.print(o); + } + }, + true); + TransferResource resource = new TransferResource(null, null, "http://maven.org/test/test-resource", null, null); + resource.setContentLength(size - 1); + + DefaultRepositorySystemSession session = new DefaultRepositorySystemSession(); + + // warm up + test(listener, session, resource, 0); + + for (int i = 1; i < size; i++) { + final int bytes = i; + + service.execute(() -> { + test(listener, session, resource, bytes); + }); + } + + // start all threads at once + try { + startLatch.await(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + + // wait for all thread to end + try { + endLatch.await(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + + StringBuilder message = new StringBuilder("Messages ["); + boolean test = true; + for (int i = 0; i < 999; i++) { + boolean ok = output.containsKey("Progress (1): test-resource (" + i + "/999 B)"); + if (!ok) { + System.out.println("false : " + i); + message.append(i + ","); + } + test = test & ok; + } + assertTrue(test, message.toString() + "] are missiong in " + output); + } + + private void test( + ConsoleMavenTransferListener listener, + DefaultRepositorySystemSession session, + TransferResource resource, + final int bytes) { + TransferEvent event = new TransferEvent.Builder(session, resource) + .setTransferredBytes(bytes) + .build(); + startLatch.countDown(); + try { + listener.transferProgressed(event); + } catch (TransferCancelledException e) { + } + endLatch.countDown(); + } +} From 2895d019c0f060180eb2215df23ee9789ca90090 Mon Sep 17 00:00:00 2001 From: sebastien-doyon Date: Thu, 19 Oct 2023 12:00:13 +0200 Subject: [PATCH 3/4] [MNG-7899] Various memory usage improvements - Non-threadsafe StringBuilder instance 'buffer' can be make class instance since it is always called in synchronized methods - remove synchronized block in transferProgressed() method, the method is synchronized and the block is not needed --- .../ConsoleMavenTransferListener.java | 55 +++++++++---------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java index be13b815b7c0..0167ff3aa0c9 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java +++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java @@ -37,6 +37,7 @@ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener private Map transfers = Collections.synchronizedMap(new LinkedHashMap<>()); private FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); // use in a synchronized fashion + private StringBuilder buffer = new StringBuilder(128); // use in a synchronized fashion private boolean printResourceNames; private int lastLength; @@ -65,39 +66,36 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer TransferResource resource = event.getResource(); transfers.put(resource, event.getTransferredBytes()); - StringBuilder buffer = new StringBuilder(128); buffer.append("Progress (").append(transfers.size()).append("): "); - synchronized (transfers) { - Iterator> entries = - transfers.entrySet().iterator(); - while (entries.hasNext()) { - Map.Entry entry = entries.next(); - long total = entry.getKey().getContentLength(); - Long complete = entry.getValue(); - - String resourceName = entry.getKey().getResourceName(); - - if (printResourceNames) { - int idx = resourceName.lastIndexOf('/'); - - if (idx < 0) { - buffer.append(resourceName); - } else { - buffer.append(resourceName, idx + 1, resourceName.length()); - } - buffer.append(" ("); - } + Iterator> entries = + transfers.entrySet().iterator(); + while (entries.hasNext()) { + Map.Entry entry = entries.next(); + long total = entry.getKey().getContentLength(); + Long complete = entry.getValue(); - buffer.append(format.formatProgress(complete, total)); + String resourceName = entry.getKey().getResourceName(); - if (printResourceNames) { - buffer.append(")"); - } + if (printResourceNames) { + int idx = resourceName.lastIndexOf('/'); - if (entries.hasNext()) { - buffer.append(" | "); + if (idx < 0) { + buffer.append(resourceName); + } else { + buffer.append(resourceName, idx + 1, resourceName.length()); } + buffer.append(" ("); + } + + buffer.append(format.formatProgress(complete, total)); + + if (printResourceNames) { + buffer.append(")"); + } + + if (entries.hasNext()) { + buffer.append(" | "); } } @@ -107,6 +105,7 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer buffer.append('\r'); out.print(buffer); out.flush(); + buffer.setLength(0); } private void pad(StringBuilder buffer, int spaces) { @@ -136,12 +135,12 @@ public synchronized void transferFailed(TransferEvent event) { private void overridePreviousTransfer(TransferEvent event) { if (lastLength > 0) { - StringBuilder buffer = new StringBuilder(lastLength + 1); pad(buffer, lastLength); buffer.append('\r'); out.print(buffer); out.flush(); lastLength = 0; + buffer.setLength(0); } } } From e3647fadcd191f17c19d79288cdb57731fd5d629 Mon Sep 17 00:00:00 2001 From: sebastien-doyon Date: Thu, 19 Oct 2023 14:29:13 +0200 Subject: [PATCH 4/4] [MNG-7899] Various memory usage improvements - Remove the Collections.synchronizedMap since all methods that use the transfers map are synchronized --- .../maven/cli/transfer/ConsoleMavenTransferListener.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java index 0167ff3aa0c9..04e8bed45fae 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java +++ b/maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java @@ -19,7 +19,6 @@ package org.apache.maven.cli.transfer; import java.io.PrintStream; -import java.util.Collections; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.Locale; @@ -35,7 +34,7 @@ */ public class ConsoleMavenTransferListener extends AbstractMavenTransferListener { - private Map transfers = Collections.synchronizedMap(new LinkedHashMap<>()); + private Map transfers = new LinkedHashMap<>(); private FileSizeFormat format = new FileSizeFormat(Locale.ENGLISH); // use in a synchronized fashion private StringBuilder buffer = new StringBuilder(128); // use in a synchronized fashion