[Patches] Small performance enhancements / coding style

[shinigami]

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.

[bastiK]

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:

-        text.append("Java version: " + System.getProperty("java.version") + ", " + System.getProperty("java.vendor") + ", " + System.getProperty("java.vm.name"));
+        text.append("Java version: ").append(System.getProperty("java.version")).append(", ").append(System.getProperty("java.vendor")).append(", ").append(System.getProperty("java.vm.name"));

...

-                if("".equals(password))
+                if(password != null && password.isEmpty())
                     password = null;

...

-                    String title = s.name + " (" + text + ")";
+                    String title = s.name + " (" + text + ')';

...

-            JList list = new JList(toPurgeAdditionally.toArray(new OsmPrimitive[0]));
+            JList list = new JList(toPurgeAdditionally.toArray(new OsmPrimitive[toPurgeAdditionally.size()]));

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.

[AlfonZ]

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;
 
         public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
-            if ("".equals(key))
+            if (key != null && key.isEmpty())
                 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
             this.key = key;
             this.value = value == null?"":value;
-            if ("".equals(this.value) && "*".equals(key)) {
+            if (this.value != null && this.value.isEmpty() && "*".equals(key)) {
                 mode = Mode.NONE;
-            } else if ("".equals(this.value)) {
+            } else if (this.value != null && this.value.isEmpty()) {
                 if (regexp) {
                     mode = Mode.MISSING_KEY_REGEXP;
                 } 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;
 
         public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
-            if ("".equals(key))
+            if (key != null && key.isEmpty())
                 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
             this.key = key;
             this.value = value == null?"":value;
-            if ("".equals(this.value) && "*".equals(key)) {
+            if (this.value.isEmpty() && "*".equals(key)) {
                 mode = Mode.NONE;
-            } else if ("".equals(this.value)) {
+            } else if (this.value.isEmpty()) {
                 if (regexp) {
                     mode = Mode.MISSING_KEY_REGEXP;
                 } else {

Maybe even to

src/org/openstreetmap/josm/actions/search/SearchCompiler.java

@@ -534 +534 @@
         private final Mode mode;
 
         public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
-            if ("".equals(key))
+            if (key.isEmpty())
                 throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
             this.key = key;
             this.value = value == null?"":value;
-            if ("".equals(this.value) && "*".equals(key)) {
+            if (this.value.isEmpty() && "*".equals(key)) {
                 mode = Mode.NONE;
-            } else if ("".equals(this.value)) {
+            } else if (this.value.isEmpty()) {
                 if (regexp) {
                     mode = Mode.MISSING_KEY_REGEXP;
                 } else {

if assumed that key being null is not valid argument here anyway and throwing NullPointerException is acceptable.

[akks]

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)

[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.

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;).

[bastiK]

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...

[shinigami]

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?

[shinigami]

replace indexOf(single char String) with indexOf(char)

[shinigami]

Efficient conversion from collection to array

[stoecker]

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.

[stoecker]

The first patch creates changes like "x != null && x != null && x.isEmpty()", which is nonsense.

[shinigami]

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..

[bastiK]

In 6083/josm:

see #8902 - Small performance enhancements (patch by shinigami)

[akks]

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)

[bastiK]

I would also like to see a changeset that adds all the remaining @Overrides in one go.

[shinigami]

added missing @Override anotations

[bastiK]

In 6084/josm:

see #8902 - add missing @Override annotations (patch by shinigami)

[shinigami]

c-like array definitions changed to java-like (int a[] -> int[] a)

[shinigami]

arrays.diff - just code style change, nice to have one style of definitions, there were few exceptions.

[shinigami]

indexOf(X) >= 0 => contains(X)

[shinigami]

code style - JLS specifies order of modifiers

[shinigami]

not-escaping local StringBuffer => StringBuilder

[shinigami]

some of string.equals("") => string.isEmpty()

[Don-vip]

In 6085/josm:

see #8902 - c-like array definitions changed to java-like (patch by shinigami)

[Don-vip]

In 6086/josm:

see #8902 - (indexOf(X) >= 0) => contains(X) (patch by shinigami)

[Don-vip]

In 6087/josm:

see #8902 - string.equals("") => string.isEmpty() (patch by shinigami)

[Don-vip]

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.

[shinigami]

code style - long literals - changed ending l to L, small l looks like 1

[shinigami]

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.

[shinigami]

str != null && str.equals("CONST") => "CONST".equals(str)

[akks]

In 6089/josm:

see #8902 - patch by shinigami
str != null && str.equals("CONST") => "CONST".equals(str)

[akks]

In 6090/josm:

see #8902 - long literals readability 4321l => 4321L (patch by shinigami)

[akks]

Nice patches, thank you!

[shinigami]

collection size ==/!= 0 -> isEmpty()/!isEmpty()

[shinigami]

effective double comparison

[shinigami]

byte[] to hex string change + small test

[stoecker]

Instead of [(v & 0xf0) >> 4] it should be [(v >> 4) & 0x0f)].

[akks]

In 6093/josm:

see #8902 - collection size ==/!= 0 -> isEmpty()/!isEmpty() (patch by shinigami)

[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.

[stoecker]

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 :-)

[shinigami]

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;

[shinigami]

new Date().getTime() => System.currentTimeMillis()

[shinigami]

while -> foreach

[shinigami]

for -> for each

[shinigami]

removed useles null checks before instanceof

[shinigami]

set init size for lists when obvious

[shinigami]

precompiled patterns

[shinigami]

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.

[akks]

I guess we should wait until stable release?

[Don-vip]

In 6102/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami):

• effective double comparison
• new Date().getTime() => System.currentTimeMillis()

[Don-vip]

In 6104/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami):

• while -> foreach
• for -> for each

plus:

• cleanup of FileDrop class to make it more integrated into JOSM core + remove warnings

[Don-vip]

In 6105/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami):

• removed useles null checks before instanceof

[Don-vip]

In 6106/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami):

• set init size for lists when obvious

[Don-vip]

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.

[shinigami]

problem in generic collection definition

[shinigami]

useless iteration to do end of list ?

[shinigami]

Environment - merge chained ops (each created copy of env.) to one

[shinigami]

compiler warnings

[shinigami]

Hi,

the last is just removing of compiler warnings. Useless ;, casts, missing type infos.

Btw. when do you plan to switch to java 7?

[Don-vip]

Hi,
The last one is not up to date I already fixed some of them in r6113 ;)
See #8465 for the Java 7 update plan.

[shinigami]

compiler warnings - against current version

[shinigami]

oki, here is patch after update to head;)

[shinigami]

Entities map is constructed eagerly and changed to {String=>Integer}, so need not be parsed again and again..

[AlfonZ]

I'm wondering, is there a reason why Entities.unescape​ is not static?

[shinigami]

map of entities

[shinigami]

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.

[shinigami]

few optimizations around osm primitives

[shinigami]

AbstractPrimitive

• getLocalName - get() is slow, need not call it twice
• hasTag - singular case of current method

Prototype

• optimized code creation

[Don-vip]

In 6142/josm:

see #8902 - fix compilation warnings

[shinigami]

index count optimization

[shinigami]

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%.

[stoecker]

Hmm, it gets confusing.

• Please open a new ticket for your next set of patches :-)
• What patches are missing in SVN?

[shinigami]

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

[Don-vip]

In 6175/josm:

see #8902 - Small performance enhancements / coding style (patches by shinigami):

• bytestohexstring.diff: byte[] to hex string change + small test
• collfix.diff : problem in generic collection definition
• styleiteration.diff: useless iteration to do end of list
• environment.diff: Environment - merge chained ops (each created copy of env.) to one

[Don-vip]

Remaining ones:

• stringbuilder.diff
• patterns.diff
• entities.diff
• primitives.diff

(quadbucketsindex.diff done in #8986)

[stoecker]

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?

[anonymous]

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.

[bastiK]

In 6821/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami)

primitives.diff: few optimizations around osm primitives

[bastiK]

In 6822/josm:

see #8902 - Small performance enhancements / coding style (patch by shinigami, updated, modified)

stringbuilder.diff: StringBuffer => StringBuilder

[bastiK]

In 6823/josm:

#8902 Small performance enhancements / coding style (patch by shinigami, parts)

patterns.diff: precompiled patterns
(only those parts applied, where performance may be relevant)

[bastiK]

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.