Replace StringBuffer with StringBuilder where possible #30964

cbuescher · 2018-05-30T13:02:19Z

StringBuilder can be used in preference to StringBuffer as it is fully
compatible but faster, since it performs no synchronization.
In the changed locations, the buffer is only accessed in local scope, so
no synchronization should be needed.

StringBuilder should be used in preference to StringBuffer as it is fully compatible but faster, as it performs no synchronization. In the changed locations the buffer is only used in local scope, so should not need synchronization.

elasticmachine · 2018-05-30T13:02:21Z

Pinging @elastic/es-core-infra

cbuescher · 2018-05-30T13:03:44Z

I don't expect this to be much of an improvement in most places, but I was just reminded about the differences by somthing I read and found it interesting to see where we use it in out codebase and can potentially change it.

nik9000 · 2018-05-30T13:20:04Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/api/Augmentation.java

@@ -474,7 +474,7 @@ public static String replaceFirst(CharSequence receiver, Pattern pattern, Functi
            // CharSequqence's toString is *supposed* to always return the characters in the sequence as a String
            return receiver.toString();
        }
-        StringBuffer result = new StringBuffer(initialBufferForReplaceWith(receiver));
+        StringBuilder result = new StringBuilder(initialBufferForReplaceWith(receiver));
        m.appendReplacement(result, Matcher.quoteReplacement(replacementBuilder.apply(m)));


This method was added in Java 9 so I don't think we can use it here until Java 8 isn't a thing for master.

For what it is worth when I wrote this the first time around I didn't see any real performance penalty for using StringBuffer. I believe the JVM can remove the uncontended synchronization fairly effectively in this case.

Makes sense, didn't know about the method. And yes, I don't see any real issue here as well, might as well keep it.

jpountz · 2018-05-30T13:40:09Z

Maybe we should consider making it and Vector forbidden APIs to prevent them from re-appearing in the future.

cbuescher · 2018-05-30T13:42:00Z

@jpountz there are legitimate usages, e.g. when having to use third party APIs that require it (e.g. Joda does in some places), so I wouldn't block it per se.

jpountz · 2018-05-30T13:45:55Z

I was thinking we could tag all methods that actually need it with @SuppressForbidden to make sure we only use those classes when there is no other choice.

droberts195 · 2018-05-30T13:47:18Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Stash.java

@@ -101,7 +101,7 @@ public Object getValue(String key) throws IOException {
         * String*Buffer* because that is what the Matcher API takes. In modern versions of java the uncontended synchronization is very,


This comment looks obsolete

Yeah, I should read more comments to save me some trouble ;-) Nik is right here of course.

jasontedor · 2018-05-30T13:55:40Z

I want to point out that this has been suggested before (this is not an argument against). For a locally allocated StringBuffer, the JVM will optimize away the locks because it knows through escape analysis that they are not needed. Therefore, we expect no improvement from this change in the case of locally allocated StringBuffers that never escape.

jasontedor · 2018-05-30T13:56:49Z

Here are my previous comments on making StringBuffer forbidden; I still think that we should not do this.

nik9000 · 2018-05-30T13:41:59Z

test/framework/src/main/java/org/elasticsearch/indices/analysis/AnalysisFactoryTestCase.java

@@ -65,7 +65,7 @@

    private static String toCamelCase(String s) {
        Matcher m = UNDERSCORE_THEN_ANYTHING.matcher(s);
-        StringBuffer sb = new StringBuffer();
+        StringBuilder sb = new StringBuilder();
        while (m.find()) {
            m.appendReplacement(sb, m.group(1).toUpperCase());


This one has the same problem with java 8.

nik9000 · 2018-05-30T13:42:21Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Stash.java

@@ -101,7 +101,7 @@ public Object getValue(String key) throws IOException {
         * String*Buffer* because that is what the Matcher API takes. In modern versions of java the uncontended synchronization is very,
         * very cheap so that should not be a problem.
         */
-        StringBuffer result = new StringBuffer(key.length());
+        StringBuilder result = new StringBuilder(key.length());


This one also has the same problem with java 8.

nik9000 · 2018-05-30T14:07:32Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Stash.java

@@ -191,7 +191,7 @@ private Object getValue(List<Object> path, String key) throws IOException {
            }
        }
        String builtPath = Matcher.quoteReplacement(pathBuilder.toString());
-        StringBuffer newKey = new StringBuffer(key.length());
+        StringBuilder newKey = new StringBuilder(key.length());


This one also uses a Java 9 method.

jpountz · 2018-05-30T14:07:38Z

To be clear, I don't worry about performance at all, only consistency.

cbuescher · 2018-05-30T14:09:02Z

@jasontedor thanks for the comment, I didn't know this already had a backstory. I just opened this mostly as an exercise while I was reading the code. I'm happy to not do this change since its nowhere near any performance-critical location.

cbuescher · 2018-05-30T14:31:28Z

The java compatibility issues leave little to change in this PR.
I'm happy to close this as well, now that it's really more Much ado bout nothing. Sorry for the noise.

nik9000

I think it'd be fine to merge it.

jasontedor

I left a comment about ActiveDirectorySIDUtil.

jasontedor · 2018-05-30T15:46:17Z

...curity/src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySIDUtil.java

@@ -38,7 +38,7 @@ static String convertToString( byte[] bytes )
        }

        char[] hex = Hex.encodeHex( bytes );
-        StringBuffer sb = new StringBuffer();
+        StringBuilder sb = new StringBuilder();


I think that we should avoid changing this source file. It is from upstream (see #30972 to fix the license) and we should not deviate unless absolutely necessary (it could get reverted in a future import from upstream anyway).

Makes sense, will close this then.

cbuescher added >non-issue review :Core/Infra/Core Core issues without another label v7.0.0 labels May 30, 2018

nik9000 reviewed May 30, 2018

View reviewed changes

Revert changes in Augmentation.java

f1a62a2

droberts195 reviewed May 30, 2018

View reviewed changes

nik9000 reviewed May 30, 2018

View reviewed changes

Some more reverts

957b82c

nik9000 approved these changes May 30, 2018

View reviewed changes

jasontedor requested changes May 30, 2018

View reviewed changes

cbuescher closed this May 30, 2018

cbuescher deleted the replace-StringBuffers branch May 30, 2018 19:08

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace StringBuffer with StringBuilder where possible #30964

Replace StringBuffer with StringBuilder where possible #30964

cbuescher commented May 30, 2018

elasticmachine commented May 30, 2018

cbuescher commented May 30, 2018 •

edited

Loading

nik9000 May 30, 2018

nik9000 May 30, 2018

cbuescher May 30, 2018

jpountz commented May 30, 2018

cbuescher commented May 30, 2018

jpountz commented May 30, 2018

droberts195 May 30, 2018

cbuescher May 30, 2018

jasontedor commented May 30, 2018

jasontedor commented May 30, 2018

nik9000 May 30, 2018

nik9000 May 30, 2018

nik9000 May 30, 2018

jpountz commented May 30, 2018

cbuescher commented May 30, 2018

cbuescher commented May 30, 2018

nik9000 left a comment

jasontedor left a comment

jasontedor May 30, 2018

cbuescher May 30, 2018

		@@ -101,7 +101,7 @@ public Object getValue(String key) throws IOException {
		* StringBuffer because that is what the Matcher API takes. In modern versions of java the uncontended synchronization is very,

Replace StringBuffer with StringBuilder where possible #30964

Replace StringBuffer with StringBuilder where possible #30964

Conversation

cbuescher commented May 30, 2018

elasticmachine commented May 30, 2018

cbuescher commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented May 30, 2018

cbuescher commented May 30, 2018

jpountz commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented May 30, 2018

jasontedor commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented May 30, 2018

cbuescher commented May 30, 2018

cbuescher commented May 30, 2018

nik9000 left a comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented May 30, 2018 •

edited

Loading