Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1772775302 @gnodet a new PR was created for that [1296](https://github.com/apache/maven/pull/1296) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1772754240 @gnodet > There are still a couple of things to clean in this package > `FileSizeFormat` should be moved to top level Done in the last commit > it should be refactored to accept an `Appender` as argument Done in the last commit, the `FileSizeFormat.formatProcess()` was refactored to accept an `StringBuilder` as argument > `BatchModeMavenTransferListener` should be removed Done in the last commit > `Slf4jMavenTransferListener` should inherit `AbstractMavenTransferListener` I leave this one for another PR, I suggest transform the `AbstractMavenTransferListener` to a message formatter class that would contain the common code building the text, and move to `PrintStream` code to the `ConsoleMavenTransferListener`. Maybe in the 1279 PR or a subsequent one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1366969590
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -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> 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));
-if (entries.hasNext()) {
-buffer.append(" | ");
+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(" (");
+}
+
+buffer.append(format.formatProgress(complete, total));
Review Comment:
Done in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet merged PR #1269: URL: https://github.com/apache/maven/pull/1269 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1366713989
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -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> 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));
-if (entries.hasNext()) {
-buffer.append(" | ");
+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(" (");
+}
+
+buffer.append(format.formatProgress(complete, total));
Review Comment:
I wonder if the `FileSizeFormat` should be refactored to be given a
`StringBuffer` to write into, instead of (or in addition to) the current
methods.
But this may be for another PR...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1365441412
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -135,7 +136,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);
Review Comment:
Good catch! I will remove the `Collections.synchronizedMap`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1365371612
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -135,7 +136,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);
Review Comment:
And also, the `transfers` field does not need to be wrapped in a
`Collections.synchronizedMap` collection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1365248238
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -135,7 +136,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);
Review Comment:
Yes this is true, I also noticed that the synchronized block at line 71 is
not needed since the transferProgressed() method is synchronized already and
synchronising on an instance var have the same effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet commented on code in PR #1269:
URL: https://github.com/apache/maven/pull/1269#discussion_r1365000724
##
maven-embedder/src/main/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener.java:
##
@@ -135,7 +136,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);
Review Comment:
This class is completely (and overly btw) synchronised, so why not using a
single `StringBuilder buffer` field and use `setLength(0)` when needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1768511456 @gnodet the last commit fixes the unit test that was broken. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1766473890 > @sebastien-doyon unit tests seem broken. @gnodet yes, I am trying to find the problem, but without access to the surefire report files, it is hard to find the problem. Do you have access or know how to get access to the target directory of a build in github? That would help debug. > Also and fwiw, this looks like all this work may be broken if we somehow integrate https://github.com/apache/maven/pull/1279... we may want to investigate on that branch then... I think the PR [1279](https://github.com/apache/maven/pull/1279) is complementary with this PR. The only change in the 1279 PR that would be needed is to add back the `MessageBuilder builder(StringBuilder stringBuilder)` method to `api/maven-api-core/src/main/java/org/apache/maven/api/services/MessageBuilderFactory.java` and `maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMessageBuilderFactory.java` classes. > Also, https://github.com/apache/maven/pull/1268, https://github.com/apache/maven/pull/1269 and https://github.com/apache/maven/pull/1270 really look related to the same goal, I.e. optimize the logging, so I think they should be merged in order to have a better understand of the benefits (even if they could be 3 different commits). If you think, I was trying to separate unrelated changes so the obvious ones could be merged while those with problems or comments could be worked out separately, like this one. Tell me if you really prefer me to merge in on PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1762920120 > @sebastien-doyon unit tests seem broken. Also and fwiw, this looks like all this work may be broken if we somehow integrate #1279... we may want to investigate on that branch then... Also, #1268, #1269 and #1270 really look related to the same goal, I.e. optimize the logging, so I think they should be merged in order to have a better understand of the benefits (even if they could be 3 different commits). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
gnodet commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1762918806 @sebastien-doyon unit tests seem broken. Also and fwiw, this looks like all this work may be broken if we somehow integrate https://github.com/apache/maven/pull/1279... we may want to investigate on that branch then... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 4 [maven]
sebastien-doyon commented on PR #1269: URL: https://github.com/apache/maven/pull/1269#issuecomment-1760038298 Memory allocation profiling using [ConsoleMavenTransferListener_memalloc_Test.java](https://github.com/sebastien-doyon/maven/blob/MNG-7899_-_4_perf-tests/maven-embedder/src/test/java/org/apache/maven/cli/transfer/ConsoleMavenTransferListener_memalloc_Test.java) calling the ConsoleMavenTransferListener class 100 million times. This test shows a drastic decrease of : - char[] allocation (before : **190 GiB**, after : **85,4 GiB**) - java.lang.String allocation (before : **45,3 GiB**, after : **3,8 GiB**) - java.lang.StringBuffer allocation (before : **65,4 GiB**, after : **1,14 GiB**) - java.text.DecimalFormat allocation (before : **27,4 GiB**, after : **none recorded**) - more... Here the JMC capture for the initial code (file available [here](https://github.com/sebastien-doyon/maven/raw/MNG-7899_-_4_perf-tests/maven-embedder/recording-initial.jfr)) https://github.com/apache/maven/assets/2573779/9ba678bd-d5bc-41c2-82d8-79f2984d880e;> Here the JMC capture for the optimised code (file available [here](https://github.com/sebastien-doyon/maven/raw/MNG-7899_-_4_perf-tests/maven-embedder/recording-optimized.jfr)) https://github.com/apache/maven/assets/2573779/525eff55-386f-4175-805e-12d8d8b02ef7;> To reproduce: - checkout the [MNG-7899-profiling-initial](https://github.com/sebastien-doyon/maven/tree/MNG-7899-profiling-initial) tag - execute the following commands : ``` mvn clean verify -Drat.skip=true -DskipTests mvn test -Drat.skip=true -pl :maven-embedder -Dtest=**/ConsoleMavenTransferListener_memalloc_Test -Dspotless.check.skip ``` - open the `maven-embedder/recording-initial.jfr` file with JMC - checkout the [MNG-7899-profiling-optimised](https://github.com/sebastien-doyon/maven/tree/MNG-7899-profiling-optimised) tag - execute the following commands : ``` mvn clean verify -Drat.skip=true -DskipTests mvn test -Drat.skip=true -pl :maven-embedder -Dtest=**/ConsoleMavenTransferListener_memalloc_Test -Dspotless.check.skip ``` - open the `maven-embedder/recording-optimized.jfr` file with JMC -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
