#8902 closed enhancement (fixed)
[Patches] Small performance enhancements / coding style
Reported by: | shinigami | Owned by: | team |
---|---|---|---|
Priority: | minor | Milestone: | |
Component: | Core | Version: | |
Keywords: | Cc: |
Description
Hi,
I am sending you patch with lots of small perf enhancements. It's based at the Idea static analysis and autocorrection, so it should not contains bugs. Changes are:
String comparison to "" => isEmpty()
coll.toArray(T[0]) -> coll.toArray(T[coll.size()])
append one char string -> append char
index of one char string -> index of char.
sb.append(s1+s2...) -> sb.append(s1).append(s2) ...
removed concats with empty strings
all are little changes but there are lots of them (and much more waiting:). I hope it will help.
Attachments (28)
Change History (85)
Changed 10 years ago by
Attachment: | perf.patch added |
---|
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Have you reviewed the changes one by one?
It seems to me, that the following one
-
src/org/openstreetmap/josm/actions/search/SearchCompiler.java
534 534 private final Mode mode; 535 535 536 536 public ExactKeyValue(boolean regexp, String key, String value) throws ParseError { 537 if ( "".equals(key))537 if (key != null && key.isEmpty()) 538 538 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value")); 539 539 this.key = key; 540 540 this.value = value == null?"":value; 541 if ( "".equals(this.value) && "*".equals(key)) {541 if (this.value != null && this.value.isEmpty() && "*".equals(key)) { 542 542 mode = Mode.NONE; 543 } else if ( "".equals(this.value)) {543 } else if (this.value != null && this.value.isEmpty()) { 544 544 if (regexp) { 545 545 mode = Mode.MISSING_KEY_REGEXP; 546 546 } else {
could be optimized even further (due to this.value
being set to non-null
value earlier).
-
src/org/openstreetmap/josm/actions/search/SearchCompiler.java
534 534 private final Mode mode; 535 535 536 536 public ExactKeyValue(boolean regexp, String key, String value) throws ParseError { 537 if ( "".equals(key))537 if (key != null && key.isEmpty()) 538 538 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value")); 539 539 this.key = key; 540 540 this.value = value == null?"":value; 541 if ( "".equals(this.value) && "*".equals(key)) {541 if (this.value.isEmpty() && "*".equals(key)) { 542 542 mode = Mode.NONE; 543 } else if ( "".equals(this.value)) {543 } else if (this.value.isEmpty()) { 544 544 if (regexp) { 545 545 mode = Mode.MISSING_KEY_REGEXP; 546 546 } else {
Maybe even to
-
src/org/openstreetmap/josm/actions/search/SearchCompiler.java
534 534 private final Mode mode; 535 535 536 536 public ExactKeyValue(boolean regexp, String key, String value) throws ParseError { 537 if ( "".equals(key))537 if (key.isEmpty()) 538 538 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value")); 539 539 this.key = key; 540 540 this.value = value == null?"":value; 541 if ( "".equals(this.value) && "*".equals(key)) {541 if (this.value.isEmpty() && "*".equals(key)) { 542 542 mode = Mode.NONE; 543 } else if ( "".equals(this.value)) {543 } else if (this.value.isEmpty()) { 544 544 if (regexp) { 545 545 mode = Mode.MISSING_KEY_REGEXP; 546 546 } else {
if assumed that key
being null
is not valid argument here anyway and throwing NullPointerException
is acceptable.
comment:3 Changed 10 years ago by
Priority: | normal → minor |
---|---|
Summary: | Small performance enhancements → [PATCH] Small performance enhancements |
comment:4 Changed 10 years ago by
Here is the performance test 'a'+ ...
vs
"a"+...
:
public class Perftest { public static void main(String[] args) { String s; long t; for (int n=0;n<5;n++) { t = System.currentTimeMillis(); for (int i=0;i < 100000000; i++) { s = "b"+Integer.toString(i)+"a"; } t=System.currentTimeMillis()-t; System.out.println("Time \"a\"= "+t); t = System.currentTimeMillis(); for (int i=0;i < 100000000; i++) { s='b'+Integer.toString(i)+'a'; } t=System.currentTimeMillis()-t; System.out.println("Time \'a\'= "+t); } } }
The result seem to show that performance effect of this particular change is extremely small (consider also problems like 'A' + 'B' == 131)
(maybe I am wrong)
comment:5 follow-up: 6 Changed 10 years ago by
Priority: | minor → normal |
---|---|
Summary: | [PATCH] Small performance enhancements → Small performance enhancements |
bastiK - yes, those are little things but there is lot of them and it sums together.
ad. toArray with empty Array argument - it creates needless object and more - result array is created by slower way.
AlfonZ - i checked it randomly, this is result of automatic processing. I know, there are LOTS OF other things to optimize, but did not wont
to make bigger changes to code yet.
Thank for feedback, I will look at code style and cut it to pieces by type, so you can choose;).
comment:6 Changed 10 years ago by
Replying to anonymous:
bastiK - yes, those are little things but there is lot of them and it sums together.
ad. toArray with empty Array argument - it creates needless object and more - result array is created by slower way.
100 x nothing = nothing :)
If it's in a performance critical section, this would be a different matter, but the hot code is at very selected spots and usually does not involve string concatenation...
comment:7 follow-up: 9 Changed 10 years ago by
Yep, and thousands times nearly nothing kill performance;).
Why to burden jvm with lots of small things, if it is so easy to give them away?
comment:8 Changed 10 years ago by
Priority: | normal → minor |
---|---|
Summary: | Small performance enhancements → [PATCH] Small performance enhancements |
Changed 10 years ago by
Attachment: | indexOf.diff added |
---|
replace indexOf(single char String) with indexOf(char)
Changed 10 years ago by
Attachment: | toArray.diff added |
---|
Efficient conversion from collection to array
comment:9 Changed 10 years ago by
Replying to shinigami:
Yep, and thousands times nearly nothing kill performance;).
Is still nothing.
Why to burden jvm with lots of small things, if it is so easy to give them away?
Because each change in the code very likely introduces new bugs. We had small optimizations which resulted in time drops from half a hour to some microseconds for certain operations, but optimizing such tiny little bits usually affects execution time in an unmeasurable way.
comment:10 follow-up: 11 Changed 10 years ago by
The first patch creates changes like "x != null && x != null && x.isEmpty()", which is nonsense.
comment:11 Changed 10 years ago by
Yes, i believed Idea too much and did not check it enough, my fault, ignore first patch.
Other two are checked fully, and it is not as much optimization as just effective idioms.
I know, that each one make small dif, but its like cleaning house, nice to clean big piles, but if everywhere is layer of dust..
comment:13 Changed 10 years ago by
I agree that indexOf.diff and toArray.diff are good for inclusion )
About other optimizations:
null-related fixing is too dangerous, fixing .append and " "+ is not very effective and bug-safe too.
isEmpty() for collections and missing @Override annotations would be good too. Can Idea do it?
(we are fixing this manually but it adds many unrelated lines to our changelog diffs)
comment:14 Changed 10 years ago by
I would also like to see a changeset that adds all the remaining @Override
s in one go.
comment:15 Changed 10 years ago by
Summary: | [PATCH] Small performance enhancements → Small performance enhancements / coding style |
---|
Changed 10 years ago by
Attachment: | arrays.diff added |
---|
c-like array definitions changed to java-like (int a[] -> int[] a)
comment:17 Changed 10 years ago by
arrays.diff - just code style change, nice to have one style of definitions, there were few exceptions.
Changed 10 years ago by
Attachment: | modifiers.diff added |
---|
code style - JLS specifies order of modifiers
Changed 10 years ago by
Attachment: | stringbuilder.diff added |
---|
not-escaping local StringBuffer => StringBuilder
Changed 10 years ago by
Attachment: | emptystring.diff added |
---|
some of string.equals("") => string.isEmpty()
comment:21 follow-up: 22 Changed 10 years ago by
Thanks for these tiny patches :)
I have committed the obvious ones, StringBuilder one needs to be reviewed carefully first. I am not sure about modifiers one as it is pure cosmetic, even if defined in JLS.
Changed 10 years ago by
Attachment: | longs.diff added |
---|
code style - long literals - changed ending l to L, small l looks like 1
comment:22 Changed 10 years ago by
You are welcome;)
Ad modifiers order, it's fine to have them always in same order, it is harder to read code if order alters.
Changed 10 years ago by
Attachment: | strcmp.diff added |
---|
str != null && str.equals("CONST") => "CONST".equals(str)
Changed 10 years ago by
Attachment: | collsizetoempty.diff added |
---|
collection size ==/!= 0 -> isEmpty()/!isEmpty()
Changed 10 years ago by
Attachment: | bytestohexstring.diff added |
---|
byte[] to hex string change + small test
comment:26 follow-up: 28 Changed 10 years ago by
Instead of [(v & 0xf0) >> 4]
it should be [(v >> 4) & 0x0f)]
.
comment:28 follow-up: 29 Changed 10 years ago by
Replying to stoecker:
Instead of
[(v & 0xf0) >> 4]
it should be[(v >> 4) & 0x0f)]
.
Why? V is int so result is same. I choose this because mask and move looks more illustrative for me.
comment:29 Changed 10 years ago by
Replying to shinigami:
Replying to stoecker:
Instead of
[(v & 0xf0) >> 4]
it should be[(v >> 4) & 0x0f)]
.
Why? V is int so result is same. I choose this because mask and move looks more illustrative for me.
Applying the 0x0f after moving ensures that for v > 255 or < 0 it does not throw exception. If it is sure, that always v >= 0 && v <= 255, then [v >> 4]
is all you need.
You're right, that result for int is equal, as &0xf0 effectively also kills any sign. Thought from programming experience I know the array access security check should always be the outermost operation :-)
comment:30 Changed 10 years ago by
I suppose the second IF should check arg1.getAction() and not arg0.getAction()
class PresetTextComparator
public int compare(JMenuItem arg0, JMenuItem arg1) {
if (Main.main.menu.presetSearchAction.equals(arg0.getAction()))
return -1;
else if (Main.main.menu.presetSearchAction.equals(arg0.getAction()))
return 1;
Changed 10 years ago by
Attachment: | datestocurrtime.diff added |
---|
new Date().getTime() => System.currentTimeMillis()
Changed 10 years ago by
Attachment: | instanceofnullcheck.diff added |
---|
removed useles null checks before instanceof
comment:31 Changed 10 years ago by
Hi, here you have a few new patches;).
The last is about regexps, it replaces matches("string"), split("string"), ... with precompiled patterns. Doesn't change single character patterns, should be optimized in jdk7.
comment:37 Changed 10 years ago by
Replying to akks:
I guess we should wait until stable release?
I have applied the obviously safe ones. For the remaining ones:
- stringbuilder.diff
- bytestohexstring.diff
- patterns.diff
I would prefer to wait for next tested, yes
For modifiers.diff, I find it too large to be included, except if it a real problem for core developers, but I don't think so.
Changed 9 years ago by
Attachment: | environment.diff added |
---|
Environment - merge chained ops (each created copy of env.) to one
comment:38 Changed 9 years ago by
Hi,
the last is just removing of compiler warnings. Useless ;, casts, missing type infos.
Btw. when do you plan to switch to java 7?
comment:39 Changed 9 years ago by
Changed 9 years ago by
Attachment: | warnings.2.diff added |
---|
compiler warnings - against current version
comment:41 follow-up: 42 Changed 9 years ago by
Entities map is constructed eagerly and changed to {String=>Integer}, so need not be parsed again and again..
comment:42 follow-up: 43 Changed 9 years ago by
I'm wondering, is there a reason why Entities.unescape is not static?
comment:43 Changed 9 years ago by
Replying to AlfonZ:
I'm wondering, is there a reason why Entities.unescape is not static?
Do not see any, there is none subclass, Entities are used as static member... I made it static and fix dependent code -> new patch version.
comment:44 Changed 9 years ago by
AbstractPrimitive
- getLocalName - get() is slow, need not call it twice
- hasTag - singular case of current method
Prototype
- optimized code creation
comment:46 Changed 9 years ago by
Hi,
I made few small changes in counting index for bbox and given level, on my testing data and in profiler it seems to run about 6-7x faster now, speedup when loading data is about 20%.
comment:47 Changed 9 years ago by
Hmm, it gets confusing.
- Please open a new ticket for your next set of patches :-)
- What patches are missing in SVN?
comment:48 Changed 9 years ago by
Oki;)
I went through comments here ant it seems this pathes are not applied:
stringbuilder.diff
bytestohexstring.diff
patterns.diff
collfix.diff
styleiteration.diff
environment.diff
entities.diff
primitives.diff
quadbucketsindex.diff
comment:49 Changed 9 years ago by
Summary: | Small performance enhancements / coding style → [Patches] Small performance enhancements / coding style |
---|
comment:51 follow-up: 57 Changed 9 years ago by
Remaining ones:
- stringbuilder.diff
- patterns.diff
- entities.diff
- primitives.diff
(quadbucketsindex.diff done in #8986)
comment:52 Changed 9 years ago by
Patterns diff sounds like a good idea to me, but why the many Javadoc extensions and why no unique naming all of them starting with PATTERN_ instead of PATTERN text sometimes front, sometimes back?
comment:53 Changed 9 years ago by
It's because I first replaced direct string usages and named them ...._PATERN. Those PATERN_... existed as string constants before, so I made PATTERN_..._COMPILED and did not renamed it.
comment:57 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Replying to Don-vip:
Remaining ones:
- stringbuilder.diff
Done.
- patterns.diff
Applied partially. It may be slightly more efficient, but I think the "inline" variant is easier to read and therefore preferred when efficiency is not an issue.
- entities.diff
I don't like to mess with a third party library for such a minor thing.
- primitives.diff
Done.
Thanks! I haven't done any benchmarks, but this patch looks as if the performance improvements are very small, probably not measurable (please prove me wrong). However, it affects the coding style and code clarity. The replacements "x" -> 'x' are mostly fine, but I don't like the following changes as they clutter the code:
Also take care to avoid unnecessary white space changes as per DevelopersGuide/PatchGuide. Could you please prepare a smaller version of your patch? Please check every line before submitting.