Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use StringBuilder instead of StringBuffer as it offers high performan… #58

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@reudismam
Copy link
Contributor

reudismam commented Feb 5, 2018

…ce in single thread places as it is generally the case.

@asfgit

This comment has been minimized.

Copy link

asfgit commented Feb 5, 2018

Can one of the admins verify this patch?

1 similar comment
@asfgit

This comment has been minimized.

Copy link

asfgit commented Feb 5, 2018

Can one of the admins verify this patch?

@@ -102,7 +102,7 @@
private static final String DEFAULT_MIME_TYPE = "text/plain";

/** Buffer in which the message is constructed prior to sending */
private StringBuffer buffer = new StringBuffer();
private StringBuilder buffer = new StringBuilder();

This comment has been minimized.

Copy link
@jaikiran

jaikiran Feb 6, 2018

Contributor

I'm not too sure this change here, in this class is correct. The StringBuffer is a thread-safe class whereas the StringBuilder isn't. Having said that I haven't checked yet whether MailLogger class is expected to be thread safe nor have I checked its usage thoroughly.

This comment has been minimized.

Copy link
@bodewig

bodewig Feb 6, 2018

Member

Given the parallel task and things like our execute framework that spawns new threads, loggers need to be thread safe, But to be honest I don't think we've ever verified MailLogger actually is.

@@ -377,7 +377,7 @@ public synchronized void run() {
}

// now did any of the threads throw an exception
exceptionMessage = new StringBuffer();
exceptionMessage = new StringBuilder();

This comment has been minimized.

Copy link
@jaikiran

jaikiran Feb 6, 2018

Contributor

Same comment as above.

This comment has been minimized.

Copy link
@bodewig

bodewig Feb 6, 2018

Member

Looking through the class I don't think exceptionMessage needs to be an instance field at all, it could be a local variable in spinThreads and get passed as an argument to processExceptions without doing any harm. To me it seems it is only ever used by a single thread.

Most probably a further refactoring could get rid of the first* instance fields as well and have processExceptions return all their values nicely encapsulated in a single object - including the accumulated messages.

@jaikiran

This comment has been minimized.

Copy link
Contributor

jaikiran commented Feb 6, 2018

Overall, IMO, this can't be a general find/replace change and instead needs to be checked for individual places where the StringBuffer is used and whether the change to StringBuilder will impact any thread safety guarantees if any.

Use StringBuilder instead of StringBuffer as it offers high performan…
…ce in single thread places as it is generally the case.

@reudismam reudismam force-pushed the reudismam:builder branch from bf3d0b4 to 1931c08 Feb 15, 2018

@reudismam

This comment has been minimized.

Copy link
Contributor Author

reudismam commented Feb 15, 2018

Undo edits to src/main/org/apache/tools/ant/listener/MailLogger.java and src/main/org/apache/tools/ant/taskdefs/Parallel.java

@bodewig

This comment has been minimized.

Copy link
Member

bodewig commented Feb 18, 2018

The changes to FixCRLF, TaskOutputStream, VerifyJar, Message, TraxLiaison and RegexpPatternMapper all change instance variables of classes that might be used in a mutlithreaded context. In the case of RegexpPatternMapper they are even protected and thus the change would break backwards compatibility.

Please restrict your change to StringBuffers created as local variables in methods that are not returned or passed to multiple threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.