[NETBEANS-6081] - Cleaning enterprise module part 4/8 #3241
Conversation
|
I have a question about the "Replace length one String in String.indexOf ("c" -> 'c')" part. Are there numbers, that show the char variant is faster or what is the reason for this change? |
enterprise/web.jsf/src/org/netbeans/modules/web/jsf/wizards/JSFClientGenerator.java
Outdated
Show resolved
Hide resolved
enterprise/web.jsf/src/org/netbeans/modules/web/jsf/wizards/JSFClientGenerator.java
Outdated
Show resolved
Hide resolved
I was trusting the |
indexOf/lastIndexOf are still slightly faster on modern JDKs if chars are used. But there is no difference anymore for StringBuilder.append which is intrinsified and should perform identical in both variants. For consistency reasons I often default to chars whenever possible - instead of single-char-Strings - so that i don't have to remember the details. |
|
Master is open for changes - it would be good to get this in early. The unittests come back green after a restart. |
|
regarding the modifications to replace concatenations with append: performance wise, this won't make any differences. a string concatenation compiled with 8 will directly compile into StringBuilder appends. On 9+ its going to use invokedynamic and java/lang/invoke/StringConcatFactory, which can be even faster. (invoke dynamic is the main reason why recompiling old libs with 9+ can actually make a difference) for readability reasons and the fact that we shouldn't actually expect any performance gain (we would miss out on the free optimization of StringConcatFactory once NB moved to 9+), i wouldn't recommend making the append changes. example: 8 9+ there is also a great talk on that topic https://www.youtube.com/watch?v=z3yu1kjtcok |
|
as a rule of thumb: Stringbuilder and append should only be used on modern JDKs if the number of concatenations is not known. E.g in loops, recursions etc. For everything else '+' gonna give best results while being also more readable. The jackpot inspection should be probably revisited at some point. I did update some inspections for 12.6, col.toArray() for example faced a similar issue: it refactored in the wrong direction, now it does the exact opposite it did before :) |
|
Hi, this is the code of my benchmarks, searching inside a string of length 52 and length 101: package javaperformance;
import java.util.concurrent.TimeUnit;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;
/**
*
* @author pepness
*/
@State(Scope.Benchmark)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@Fork(3)
public class IndexOfTest {
private String str;
private char c;
private String s;
@Setup
public void setup() {
// str = " !\"#$%&\\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"; // length 101
str = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; // length 52
s = "z";
c = 'z';
}
@Benchmark
public int indexOfChar() {
return str.indexOf('z');
}
@Benchmark
public int indexOfString() {
return str.indexOf("z");
}
@Benchmark
public int lastIndexOfChar() {
return str.lastIndexOf('z');
}
@Benchmark
public int lastIndexOfString() {
return str.lastIndexOf("z");
}
@Benchmark
public int indirectIndexOfChar() {
return str.indexOf(c);
}
@Benchmark
public int indirectIndexOfString() {
return str.indexOf(s);
}
@Benchmark
public int indirectLastIndexOfChar() {
return str.indexOf(c);
}
@Benchmark
public int indirectLastIndexOfString() {
return str.indexOf(s);
}
public static void main(String[] args) throws RunnerException {
Options opt = new OptionsBuilder()
.include(IndexOfTest.class.getSimpleName())
.threads(1)
.build();
new Runner(opt).run();
}
}These are the results: JMH version: 1.33 Search inside a string of length 52 Search inside a string of length 101 VM version: JDK 1.8.0_312, OpenJDK 64-Bit Server VM, 25.312-b07 Search inside a string of length 52 Search inside a string of length 101 VM version: JDK 11.0.13, OpenJDK 64-Bit Server VM, 11.0.13+8-LTS Search inside a string of length 52 Search inside a string of length 101 VM version: JDK 17.0.1, OpenJDK 64-Bit Server VM, 17.0.1+12-LTS Search inside a string of length 52 Search inside a string of length 101 According to my testing (it could be wrong, hope you can test it) when using indexOf/lastIndexOf with chars is up to 53% faster using the latest LTS JDK 17+ but it is slower with previous LTS JDK 11 up to 93%. When using JDK 7 or 8 the performance difference is negligible at least at a length of 101 or less. If NetBeans is going to be compiled using JDK 11 I would recommend using "c" instead of 'c'. |
|
@pepness the benchmark looks solid, thanks for sharing. It matches my results too using other JMH benchmarks I wrote once. (I usually have "time" set to 5s instead of 500ms to give the JIT a bit more iterations) a few remarks:
My take on this: Since indexof is not dependent on the javac language level, lets optimize for latest JDK versions. The difference is very small but the code difference is tiny too. If 'c' compared to "c" is a little bit faster, its hard to find reasons to not use it. Even if the performance is going to be equal one day (as it is with replace, append etc), it wouldn't cause a regression either - not in code style or performance. [1] javac is translating it to standard You can check this with |
|
results on my system using your benchmark, 52 char string, 5s cycle time. |
|
looking good. thanks for the changes. |
| getIdBody.append("String params[] = new String[" + params + "];\n" + | ||
| "int p = 0;\n" + | ||
| "int grabStart = 0;\n" + | ||
| "String delim = \"#\";\n" + | ||
| "String escape = \"~\";\n" + | ||
| "Pattern pattern = Pattern.compile(escape + \"*\" + delim);\n" + | ||
| "Matcher matcher = pattern.matcher(string);\n" + | ||
| "while (matcher.find()) {\n" + | ||
| "String found = matcher.group();\n" + | ||
| "if (found.length() % 2 == 1) {\n" + | ||
| "params[p] = string.substring(grabStart, matcher.start());\n" + | ||
| "p++;\n" + | ||
| "grabStart = matcher.end();\n" + | ||
| "}\n" + | ||
| "}\n" + | ||
| "if (p != params.length - 1) {\n" + | ||
| "throw new IllegalArgumentException(\"string \" + string + \" is not in expected format. expected " + params + " ids delimited by \" + delim);\n" + | ||
| "}\n" + | ||
| "params[p] = string.substring(grabStart);\n" + | ||
| "for (int i = 0; i < params.length; i++) {\n" + | ||
| "params[i] = params[i].replace(escape + delim, delim);\n" + | ||
| "params[i] = params[i].replace(escape + escape, escape);\n" + | ||
| "}\n\n" | ||
| ); | ||
| getIdBody.append("String params[] = new String[").append(params).append("];\n") | ||
| .append("int p = 0;\n") | ||
| .append("int grabStart = 0;\n") | ||
| .append("String delim = \"#\";\n") | ||
| .append("String escape = \"~\";\n") | ||
| .append("Pattern pattern = Pattern.compile(escape + \"*\" + delim);\n") | ||
| .append("Matcher matcher = pattern.matcher(string);\n") | ||
| .append("while (matcher.find()) {\n") | ||
| .append("String found = matcher.group();\n") | ||
| .append("if (found.length() % 2 == 1) {\n") | ||
| .append("params[p] = string.substring(grabStart, matcher.start());\n") | ||
| .append("p++;\n") | ||
| .append("grabStart = matcher.end();\n") | ||
| .append("}\n") | ||
| .append("}\n") | ||
| .append("if (p != params.length - 1) {\n") | ||
| .append("throw new IllegalArgumentException(\"string \" + string + \" is not in expected format. expected ").append(params).append(" ids delimited by \" + delim);\n") | ||
| .append("}\n") | ||
| .append("params[p] = string.substring(grabStart);\n") | ||
| .append("for (int i = 0; i < params.length; i++) {\n") | ||
| .append("params[i] = params[i].replace(escape + delim, delim);\n") | ||
| .append("params[i] = params[i].replace(escape + escape, escape);\n") | ||
| .append("}\n\n"); |
There was a problem hiding this comment.
i am not a big fan of those kind of changes. Readability wise, both are similar, but performance wise the original version is going to win. Simple concatenations with constants "foo"+"bar" becomes "foobar" in bytecode anyway, rest is done via invoke dynamic and is likely to be faster than append.
Looking forward, it is also easier to convert concatenations to text blocks in future, which would have a readability gain without performance penalty. Or even later to string templates. append chains are a step back in that regard.
sb.append() is fine for loops or situations where the String structure is not known.
There was a problem hiding this comment.
True.
My initial concern was for the readabiility of the automatic cleanup. Future features for the moment are that, future features. Until then my preference is that I'd rather read a multiline version of code/pseudo code like this instead of a single line string hundreds of characters long.
Had we had textblocks available I'd have preferred that even over the current state of the change, but we don't.
There was a problem hiding this comment.
so then why change it? Is it a cleanup when there is no improvement in readability while having a performance cost (even though performance doesn't matter at all in this particular case, its not in a inner loop or anything like that).
There was a problem hiding this comment.
lets agree at least to not run the append refactoring for future cleanups - its a legacy code inspection from java 1.5 times.
But it would be good if the big text block candidates could be reverted back to the original version, so that they can be easier converted in future (NB has a refactoring for that but not for append chains).
| getAsStringBody.append(idPropertyType[0] + " id = o." + idGetterName[0] + "();\n" + | ||
| "if (id == null) {\n" + | ||
| "return \"\";\n" + | ||
| "}\n" + | ||
| "String delim = \"#\";\n" + | ||
| "String escape = \"~\";\n\n" | ||
| ); | ||
| getAsStringBody.append(idPropertyType[0]).append(" id = o.").append(idGetterName[0]).append("();\n") | ||
| .append("if (id == null) {\n") | ||
| .append("return \"\";\n") | ||
| .append("}\n") | ||
| .append("String delim = \"#\";\n") | ||
| .append("String escape = \"~\";\n\n"); |
| org.netbeans.modules.schema2beans.BaseBean n; | ||
| str.append(indent); | ||
| str.append("FileEntry["+this.sizeFileEntry()+"]"); // NOI18N | ||
| str.append("FileEntry[").append(this.sizeFileEntry()).append("]"); // NOI18N |
There was a problem hiding this comment.
changes like this are really unnecessary. Since it would compile to append anyway on 8, and the faster invokedynamic+StringConcatFactory version on 9+. It doesn't improve readability and might reduce performance -> lets not change it ;)
explained here #3241 (comment)
|
Things that need addressing here so punting to NB14. |
- Replace length one String in String.indexOf with character - Remove redundant String.toString() - Replace String concatenation with '.append' when using StringBuilder - Remove String constructors - Remove unnecessary temporary during conversion from String - Remove unnecessary temporary during conversion to String - Remove useless use of StringBuffer with StringBuilder
…FClientGenerator.java Improve readability Co-authored-by: Martin Klähn <mklaehn@users.noreply.github.com>
…FClientGenerator.java Improve readability Co-authored-by: Martin Klähn <mklaehn@users.noreply.github.com>
pepness
force-pushed
the
cleaning-enterprise-4
branch
from
7acf026 to
020d159
Compare
|
Travis job "Check line endings and verify RAT report" fails on unrelated empty new file enterprise/micronaut/src/org/netbeans/modules/micronaut/completion/MicronautConfigCompletionItem.java |
|
Thank you @mklaehn, I mess up the rebase, will keep working on it. |
|
Given this is pushing through milestones without resolution, closing. Please re-open if/when resolved. |
5 participants
I did some cleaning to the
enterprisemodule with theInspect & Transformtool. I divided the cleaning in 8 parts/commits. This is the 4th PR.Cleaning enterprise module part 4:
appendwhen using StringBuilder/StringBuffernew String()I did full builds and fix the errors that pop out.
NetBeans Testing:
enterpriseand compare the output to the same unit test from master