Skip to content

Commit

Permalink
[MNG-7899] Various memory usage improvements 4 (#1269)
Browse files Browse the repository at this point in the history
- 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
- 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
- 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
- Remove the Collections.synchronizedMap since all methods that use the transfers map are synchronized
  • Loading branch information
sebastien-doyon authored Oct 20, 2023
1 parent 9abaf3a commit 68bbc8f
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -35,7 +34,9 @@
*/
public class ConsoleMavenTransferListener extends AbstractMavenTransferListener {

private Map<TransferResource, Long> transfers = Collections.synchronizedMap(new LinkedHashMap<>());
private Map<TransferResource, Long> 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

private boolean printResourceNames;
private int lastLength;
Expand Down Expand Up @@ -64,20 +65,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<Map.Entry<TransferResource, Long>> entries =
transfers.entrySet().iterator();
while (entries.hasNext()) {
Map.Entry<TransferResource, Long> entry = entries.next();
long total = entry.getKey().getContentLength();
Long complete = entry.getValue();
buffer.append(getStatus(entry.getKey().getResourceName(), complete, total));
if (entries.hasNext()) {
buffer.append(" | ");
Iterator<Map.Entry<TransferResource, Long>> entries =
transfers.entrySet().iterator();
while (entries.hasNext()) {
Map.Entry<TransferResource, Long> 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(" (");
}

buffer.append(format.formatProgress(complete, total));

if (printResourceNames) {
buffer.append(")");
}

if (entries.hasNext()) {
buffer.append(" | ");
}
}

Expand All @@ -87,25 +104,7 @@ public synchronized void transferProgressed(TransferEvent event) throws Transfer
buffer.append('\r');
out.print(buffer);
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();
buffer.setLength(0);
}

private void pad(StringBuilder buffer, int spaces) {
Expand Down Expand Up @@ -135,12 +134,12 @@ public synchronized void transferFailed(TransferEvent event) {

private void overridePreviousTransfer(TransferEvent event) {
if (lastLength > 0) {
StringBuilder buffer = new StringBuilder(128);
pad(buffer, lastLength);
buffer.append('\r');
out.print(buffer);
out.flush();
lastLength = 0;
buffer.setLength(0);
}
}
}
Original file line number Diff line number Diff line change
@@ -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<String, String> output = new ConcurrentHashMap<String, String>();

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();
}
}

0 comments on commit 68bbc8f

Please sign in to comment.