You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by reudismam <gi...@git.apache.org> on 2018/02/05 16:41:02 UTC
[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...
GitHub user reudismam opened a pull request:
https://github.com/apache/ant/pull/58
Use StringBuilder instead of StringBuffer as it offers high performan…
…ce in single thread places as it is generally the case.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/reudismam/ant builder
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/ant/pull/58.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #58
----
commit bf3d0b448ed055eea7dcc036397e8b0f7cbc50fe
Author: reudismam <re...@...>
Date: 2018-02-05T16:18:05Z
Use StringBuilder instead of StringBuffer as it offers high performance in single thread places as it is generally the case.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:
https://github.com/apache/ant/pull/58#discussion_r166183611
--- Diff: src/main/org/apache/tools/ant/listener/MailLogger.java ---
@@ -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();
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:
https://github.com/apache/ant/pull/58#discussion_r166217627
--- Diff: src/main/org/apache/tools/ant/taskdefs/Parallel.java ---
@@ -377,7 +377,7 @@ public synchronized void run() {
}
// now did any of the threads throw an exception
- exceptionMessage = new StringBuffer();
+ exceptionMessage = new StringBuilder();
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on a diff in the pull request:
https://github.com/apache/ant/pull/58#discussion_r166183671
--- Diff: src/main/org/apache/tools/ant/taskdefs/Parallel.java ---
@@ -377,7 +377,7 @@ public synchronized void run() {
}
// now did any of the threads throw an exception
- exceptionMessage = new StringBuffer();
+ exceptionMessage = new StringBuilder();
--- End diff --
Same comment as above.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:
https://github.com/apache/ant/pull/58#discussion_r166216151
--- Diff: src/main/org/apache/tools/ant/listener/MailLogger.java ---
@@ -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();
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/ant/pull/58
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 `StringBuffer`s created as local variables in methods that are not returned or passed to multiple threads.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/ant/pull/58
Undo edits to src/main/org/apache/tools/ant/listener/MailLogger.java and src/main/org/apache/tools/ant/taskdefs/Parallel.java
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/ant/pull/58
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...
Posted by jaikiran <gi...@git.apache.org>.
Github user jaikiran commented on the issue:
https://github.com/apache/ant/pull/58
Overall, IMO, this can't be a general find/replace change and instead needs to be checked for individuals places where the `StringBuffer` is used and whether the change to `StringBuilder` will impact any thread safety guarantees if any.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org