[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

bodewig
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 <reudismam@...>
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...

bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #58: Use StringBuilder instead of StringBuffer as it offers...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #58: Use StringBuilder instead of StringBuffer as it offers high p...

bodewig
In reply to this post by bodewig
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: [hidden email]
For additional commands, e-mail: [hidden email]