You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "sebastien-doyon (via GitHub)" <gi...@apache.org> on 2023/09/26 15:15:58 UTC
[GitHub] [maven] sebastien-doyon opened a new pull request, #1270: [MNG-7899] Various memory usage improvements
sebastien-doyon opened a new pull request, #1270:
URL: https://github.com/apache/maven/pull/1270
Multiple optimizations :
- renderLevel() method use static constants instead of rebuilding the
strings on each call
- replace + operator usage with more PrintStream.print() calls to reduce
temporary strings creation
- reduce usage of MessageBuilder.a() method usage with more
PrintStream.print() calls to reduce temporary strings creation
- replace the builder() method with a static import
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1365467245
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -30,6 +29,14 @@
* @since 3.5.0
*/
public class MavenSimpleLogger extends SimpleLogger {
+
+ private static final String TRACE_RENDERED_LEVEL = builder().trace("TRACE").build();
+ private static final String DEBUG_RENDERED_LEVEL = builder().debug("DEBUG").build();
+ private static final String INFO_RENDERED_LEVEL = builder().info("INFO").build();
+ private static final String WARN_RENDERED_LEVEL =
+ builder().warning("WARNING").build();
+ private static final String ERROR_RENDERED_LEVEL = builder().error("ERROR").build();
Review Comment:
I see your point, I will try to find and propose a better solution a better solution. I would like to avoid recreating those strings on each `MavenSimpleLogger` instances. I know that the usual use case is that there is only on instance of `MavenSimpleLogger`, the root logger, but it is also possible to configure loggers per component or class with more efforts.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1365453818
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -71,8 +78,13 @@ private void printStackTrace(Throwable t, PrintStream stream, String prefix) {
stream.print(prefix);
stream.print(" ");
stream.print(builder().strong("at"));
- stream.print(" " + e.getClassName() + "." + e.getMethodName());
- stream.print(builder().a(" (").strong(getLocation(e)).a(")"));
+ stream.print(" ");
+ stream.print(e.getClassName());
+ stream.print(".");
+ stream.print(e.getMethodName());
+ stream.print(" (");
+ stream.print(builder().strong(getLocation(e)));
+ stream.print(")");
Review Comment:
Yes that is right, I noticed this while doing different tests but forgot to push the changes. Pushing the commit now.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1365012668
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -71,8 +78,13 @@ private void printStackTrace(Throwable t, PrintStream stream, String prefix) {
stream.print(prefix);
stream.print(" ");
stream.print(builder().strong("at"));
- stream.print(" " + e.getClassName() + "." + e.getMethodName());
- stream.print(builder().a(" (").strong(getLocation(e)).a(")"));
+ stream.print(" ");
+ stream.print(e.getClassName());
+ stream.print(".");
+ stream.print(e.getMethodName());
+ stream.print(" (");
+ stream.print(builder().strong(getLocation(e)));
+ stream.print(")");
Review Comment:
Are you sure this is interesting in terms of performances ? The benefit to avoid multiple buffer allocations may be balanced by the synchronisation / flushes on the underlying character/byte streams...
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -30,6 +29,14 @@
* @since 3.5.0
*/
public class MavenSimpleLogger extends SimpleLogger {
+
+ private static final String TRACE_RENDERED_LEVEL = builder().trace("TRACE").build();
+ private static final String DEBUG_RENDERED_LEVEL = builder().debug("DEBUG").build();
+ private static final String INFO_RENDERED_LEVEL = builder().info("INFO").build();
+ private static final String WARN_RENDERED_LEVEL =
+ builder().warning("WARNING").build();
+ private static final String ERROR_RENDERED_LEVEL = builder().error("ERROR").build();
Review Comment:
I'd rather have those as fields rather than static. In environments such as `mvnd`, this would avoid leaking system properties between builds.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1270: [MNG-7899] Various memory usage improvements 5
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1340453474
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -495,19 +495,6 @@ void logging(CliRequest cliRequest) {
cliRequest.quiet = !cliRequest.verbose && commandLine.hasOption(CLIManager.QUIET);
cliRequest.showErrors = cliRequest.verbose || commandLine.hasOption(CLIManager.ERRORS);
- slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
Review Comment:
Well, the MessageUtils.setColorEnabled must be called before calling slf4jConfiguration.setRootLoggerLevel witch load for the first time the MavenSimpleLogger class. Since this modified class use now static constants, its is necessary that the colour support is set right before loading the MavenSimpleLogger class.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1366754120
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/MessageBuilder.java:
##########
@@ -206,4 +206,11 @@ public interface MessageBuilder {
*/
@Nonnull
String build();
+
+ /**
+ * Set the buffer length.
+ *
+ * @param length the new length
+ */
+ void setLength(int length);
Review Comment:
Shouldn't this be reused in #1269 somehow ? I mean that PR mixes ansi formatting with string builders... No big deal for now though.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] sebastien-doyon commented on a diff in pull request #1270: [MNG-7899] Various memory usage improvements 5
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1340453474
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -495,19 +495,6 @@ void logging(CliRequest cliRequest) {
cliRequest.quiet = !cliRequest.verbose && commandLine.hasOption(CLIManager.QUIET);
cliRequest.showErrors = cliRequest.verbose || commandLine.hasOption(CLIManager.ERRORS);
- slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
Review Comment:
Well, the `MessageUtils.setColorEnabled` must be called before calling `slf4jConfiguration.setRootLoggerLevel` witch load for the first time the `MavenSimpleLogger` class. Since this modified class use now static constants, its is necessary that the colour support is set right before loading the `MavenSimpleLogger` class.
This was exposed by tests failing in the `core-it-suite`
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a diff in pull request #1270: [MNG-7899] Various memory usage improvements 5
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1340193426
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -495,19 +495,6 @@ void logging(CliRequest cliRequest) {
cliRequest.quiet = !cliRequest.verbose && commandLine.hasOption(CLIManager.QUIET);
cliRequest.showErrors = cliRequest.verbose || commandLine.hasOption(CLIManager.ERRORS);
- slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
Review Comment:
Moving this code block is necessary because Jansi needs to be initialised before the execution of this block when `MessageUtils.setColorEnabled` is called ?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1367034601
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/MessageBuilder.java:
##########
@@ -206,4 +206,11 @@ public interface MessageBuilder {
*/
@Nonnull
String build();
+
+ /**
+ * Set the buffer length.
+ *
+ * @param length the new length
+ */
+ void setLength(int length);
Review Comment:
For me it is not related, the usage of the `MessageUtils` class is not the same
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1366679339
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -30,6 +29,14 @@
* @since 3.5.0
*/
public class MavenSimpleLogger extends SimpleLogger {
+
+ private static final String TRACE_RENDERED_LEVEL = builder().trace("TRACE").build();
+ private static final String DEBUG_RENDERED_LEVEL = builder().debug("DEBUG").build();
+ private static final String INFO_RENDERED_LEVEL = builder().info("INFO").build();
+ private static final String WARN_RENDERED_LEVEL =
+ builder().warning("WARNING").build();
+ private static final String ERROR_RENDERED_LEVEL = builder().error("ERROR").build();
Review Comment:
I've changed the static constants to class member as a temporary solution until the [1279](https://github.com/apache/maven/pull/1279) PR is merged.
I also changed the StringBuilder use to MessageBuilder to facilitate the merge with the PR, the JLine implementation
would not support providing a StringBuilder as the internal buffer. To Enable reusing the MessageBuilder, a new method `setLength(int)` was added.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1366758647
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -103,13 +117,9 @@ protected String getLocation(final StackTraceElement e) {
} else if (e.getFileName() == null) {
return "Unknown Source";
} else if (e.getLineNumber() >= 0) {
- return String.format("%s:%s", e.getFileName(), e.getLineNumber());
+ return e.getFileName() + ":" + e.getLineNumber();
Review Comment:
Shouldn't we go further and pass the message builder as an argument to avoid concatenating strings here ?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1367045641
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -103,13 +117,9 @@ protected String getLocation(final StackTraceElement e) {
} else if (e.getFileName() == null) {
return "Unknown Source";
} else if (e.getLineNumber() >= 0) {
- return String.format("%s:%s", e.getFileName(), e.getLineNumber());
+ return e.getFileName() + ":" + e.getLineNumber();
Review Comment:
I think it is not worth the trouble, in the case where the getLocation method would concatenate the file name and the lin e number, we would need to surround `e.getFileName() + ":" + e.getLineNumber()` with the strong style, separately or with a style set and reset. I see no support for that in the current implementation (style set/reset).
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "sebastien-doyon (via GitHub)" <gi...@apache.org>.
sebastien-doyon commented on code in PR #1270:
URL: https://github.com/apache/maven/pull/1270#discussion_r1367046791
##########
maven-slf4j-provider/src/main/java/org/slf4j/impl/MavenSimpleLogger.java:
##########
@@ -103,13 +117,9 @@ protected String getLocation(final StackTraceElement e) {
} else if (e.getFileName() == null) {
return "Unknown Source";
} else if (e.getLineNumber() >= 0) {
- return String.format("%s:%s", e.getFileName(), e.getLineNumber());
+ return e.getFileName() + ":" + e.getLineNumber();
Review Comment:
feel free to correct me if I am wrong....
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-7899] Various memory usage improvements 5 [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #1270:
URL: https://github.com/apache/maven/pull/1270
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org