#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)
by , 12 years ago
Attachment: | perf.patch added |
---|
comment:1 by , 12 years ago
comment:2 by , 12 years ago
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 by , 12 years ago
Priority: | normal → minor |
---|---|
Summary: | Small performance enhancements → [PATCH] Small performance enhancements |
comment:4 by , 12 years ago
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)
follow-up: 6 comment:5 by , 12 years ago
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 by , 12 years ago
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...
follow-up: 9 comment:7 by , 12 years ago
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 by , 12 years ago
Priority: | normal → minor |
---|---|
Summary: | Small performance enhancements → [PATCH] Small performance enhancements |
by , 12 years ago
Attachment: | indexOf.diff added |
---|
replace indexOf(single char String) with indexOf(char)
comment:9 by , 12 years ago
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.
follow-up: 11 comment:10 by , 12 years ago
The first patch creates changes like "x != null && x != null && x.isEmpty()", which is nonsense.
comment:11 by , 12 years ago
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 by , 12 years ago
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 by , 12 years ago
I would also like to see a changeset that adds all the remaining @Override
s in one go.
comment:15 by , 12 years ago
Summary: | [PATCH] Small performance enhancements → Small performance enhancements / coding style |
---|
by , 12 years ago
Attachment: | arrays.diff added |
---|
c-like array definitions changed to java-like (int a[] -> int[] a)
comment:17 by , 12 years ago
arrays.diff - just code style change, nice to have one style of definitions, there were few exceptions.
by , 12 years ago
Attachment: | stringbuilder.diff added |
---|
not-escaping local StringBuffer => StringBuilder
follow-up: 22 comment:21 by , 12 years ago
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.
by , 12 years ago
Attachment: | longs.diff added |
---|
code style - long literals - changed ending l to L, small l looks like 1
comment:22 by , 12 years ago
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.
by , 12 years ago
Attachment: | strcmp.diff added |
---|
str != null && str.equals("CONST") => "CONST".equals(str)
by , 12 years ago
Attachment: | collsizetoempty.diff added |
---|
collection size ==/!= 0 -> isEmpty()/!isEmpty()
follow-up: 28 comment:26 by , 12 years ago
Instead of [(v & 0xf0) >> 4]
it should be [(v >> 4) & 0x0f)]
.
follow-up: 29 comment:28 by , 12 years ago
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 by , 12 years ago
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 by , 12 years ago
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;
by , 12 years ago
Attachment: | datestocurrtime.diff added |
---|
new Date().getTime() => System.currentTimeMillis()
by , 12 years ago
Attachment: | instanceofnullcheck.diff added |
---|
removed useles null checks before instanceof
comment:31 by , 11 years ago
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 by , 11 years ago
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.
by , 11 years ago
Attachment: | environment.diff added |
---|
Environment - merge chained ops (each created copy of env.) to one
comment:38 by , 11 years ago
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 by , 11 years ago
follow-up: 42 comment:41 by , 11 years ago
Entities map is constructed eagerly and changed to {String=>Integer}, so need not be parsed again and again..
follow-up: 43 comment:42 by , 11 years ago
I'm wondering, is there a reason why Entities.unescape is not static?
comment:43 by , 11 years ago
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 by , 11 years ago
AbstractPrimitive
- getLocalName - get() is slow, need not call it twice
- hasTag - singular case of current method
Prototype
- optimized code creation
comment:46 by , 11 years ago
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 by , 11 years ago
Hmm, it gets confusing.
- Please open a new ticket for your next set of patches :-)
- What patches are missing in SVN?
comment:48 by , 11 years ago
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 by , 11 years ago
Summary: | Small performance enhancements / coding style → [Patches] Small performance enhancements / coding style |
---|
follow-up: 57 comment:51 by , 11 years ago
Remaining ones:
- stringbuilder.diff
- patterns.diff
- entities.diff
- primitives.diff
(quadbucketsindex.diff done in #8986)
comment:52 by , 11 years ago
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 by , 11 years ago
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 by , 11 years ago
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.