Modify

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

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

perf.patch (242.7 KB) - added by shinigami 6 years ago.
indexOf.diff (10.5 KB) - added by shinigami 6 years ago.
replace indexOf(single char String) with indexOf(char)
toArray.diff (4.9 KB) - added by shinigami 6 years ago.
Efficient conversion from collection to array
override.diff (387.8 KB) - added by shinigami 6 years ago.
added missing @Override anotations
arrays.diff (18.5 KB) - added by shinigami 6 years ago.
c-like array definitions changed to java-like (int a[] -> int[] a)
contains.diff (590 bytes) - added by shinigami 6 years ago.
indexOf(X) >= 0 => contains(X)
modifiers.diff (224.0 KB) - added by shinigami 6 years ago.
code style - JLS specifies order of modifiers
stringbuilder.diff (24.2 KB) - added by shinigami 6 years ago.
not-escaping local StringBuffer => StringBuilder
emptystring.diff (44.1 KB) - added by shinigami 6 years ago.
some of string.equals("") => string.isEmpty()
longs.diff (5.3 KB) - added by shinigami 6 years ago.
code style - long literals - changed ending l to L, small l looks like 1
strcmp.diff (2.9 KB) - added by shinigami 6 years ago.
str != null && str.equals("CONST") => "CONST".equals(str)
collsizetoempty.diff (32.7 KB) - added by shinigami 6 years ago.
collection size ==/!= 0 -> isEmpty()/!isEmpty()
doublecmp.diff (600 bytes) - added by shinigami 6 years ago.
effective double comparison
bytestohexstring.diff (2.6 KB) - added by shinigami 6 years ago.
byte[] to hex string change + small test
datestocurrtime.diff (4.7 KB) - added by shinigami 6 years ago.
new Date().getTime() => System.currentTimeMillis()
whiletoforeach.diff (4.9 KB) - added by shinigami 6 years ago.
while -> foreach
fortoforeach.diff (25.1 KB) - added by shinigami 6 years ago.
for -> for each
instanceofnullcheck.diff (9.0 KB) - added by shinigami 6 years ago.
removed useles null checks before instanceof
initsizes.diff (10.1 KB) - added by shinigami 6 years ago.
set init size for lists when obvious
patterns.diff (67.0 KB) - added by shinigami 6 years ago.
precompiled patterns
collfix.diff (628 bytes) - added by shinigami 6 years ago.
problem in generic collection definition
styleiteration.diff (675 bytes) - added by shinigami 6 years ago.
useless iteration to do end of list ?
environment.diff (4.3 KB) - added by shinigami 6 years ago.
Environment - merge chained ops (each created copy of env.) to one
warnings.diff (24.7 KB) - added by shinigami 6 years ago.
compiler warnings
warnings.2.diff (24.2 KB) - added by shinigami 6 years ago.
compiler warnings - against current version
entities.diff (3.3 KB) - added by shinigami 6 years ago.
map of entities
primitives.diff (4.3 KB) - added by shinigami 6 years ago.
few optimizations around osm primitives
quadbucketsindex.diff (5.1 KB) - added by shinigami 6 years ago.
index count optimization

Download all attachments as: .zip

Change History (85)

Changed 6 years ago by shinigami

Attachment: perf.patch added

comment:1 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by 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

     
    534534        private final Mode mode;
    535535
    536536        public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
    537             if ("".equals(key))
     537            if (key != null && key.isEmpty())
    538538                throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
    539539            this.key = key;
    540540            this.value = value == null?"":value;
    541             if ("".equals(this.value) && "*".equals(key)) {
     541            if (this.value != null && this.value.isEmpty() && "*".equals(key)) {
    542542                mode = Mode.NONE;
    543             } else if ("".equals(this.value)) {
     543            } else if (this.value != null && this.value.isEmpty()) {
    544544                if (regexp) {
    545545                    mode = Mode.MISSING_KEY_REGEXP;
    546546                } 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

     
    534534        private final Mode mode;
    535535
    536536        public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
    537             if ("".equals(key))
     537            if (key != null && key.isEmpty())
    538538                throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
    539539            this.key = key;
    540540            this.value = value == null?"":value;
    541             if ("".equals(this.value) && "*".equals(key)) {
     541            if (this.value.isEmpty() && "*".equals(key)) {
    542542                mode = Mode.NONE;
    543             } else if ("".equals(this.value)) {
     543            } else if (this.value.isEmpty()) {
    544544                if (regexp) {
    545545                    mode = Mode.MISSING_KEY_REGEXP;
    546546                } else {

Maybe even to

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

     
    534534        private final Mode mode;
    535535
    536536        public ExactKeyValue(boolean regexp, String key, String value) throws ParseError {
    537             if ("".equals(key))
     537            if (key.isEmpty())
    538538                throw new ParseError(tr("Key cannot be empty when tag operator is used. Sample use: key=value"));
    539539            this.key = key;
    540540            this.value = value == null?"":value;
    541             if ("".equals(this.value) && "*".equals(key)) {
     541            if (this.value.isEmpty() && "*".equals(key)) {
    542542                mode = Mode.NONE;
    543             } else if ("".equals(this.value)) {
     543            } else if (this.value.isEmpty()) {
    544544                if (regexp) {
    545545                    mode = Mode.MISSING_KEY_REGEXP;
    546546                } else {

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

comment:3 Changed 6 years ago by Don-vip

Priority: normalminor
Summary: Small performance enhancements[PATCH] Small performance enhancements

comment:4 Changed 6 years ago by 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)

comment:5 Changed 6 years ago by anonymous

Priority: minornormal
Summary: [PATCH] Small performance enhancementsSmall 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 in reply to:  5 Changed 6 years ago by 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...

comment:7 Changed 6 years ago by 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?

comment:8 Changed 6 years ago by Don-vip

Priority: normalminor
Summary: Small performance enhancements[PATCH] Small performance enhancements

Changed 6 years ago by shinigami

Attachment: indexOf.diff added

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

Changed 6 years ago by shinigami

Attachment: toArray.diff added

Efficient conversion from collection to array

comment:9 in reply to:  7 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by stoecker

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

comment:11 in reply to:  10 Changed 6 years ago by 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..

comment:12 Changed 6 years ago by bastiK

In 6083/josm:

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

comment:13 Changed 6 years ago by 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)

comment:14 Changed 6 years ago by bastiK

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

comment:15 Changed 6 years ago by bastiK

Summary: [PATCH] Small performance enhancementsSmall performance enhancements / coding style

Changed 6 years ago by shinigami

Attachment: override.diff added

added missing @Override anotations

comment:16 Changed 6 years ago by bastiK

In 6084/josm:

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

Changed 6 years ago by shinigami

Attachment: arrays.diff added

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

comment:17 Changed 6 years ago by shinigami

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

Changed 6 years ago by shinigami

Attachment: contains.diff added

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

Changed 6 years ago by shinigami

Attachment: modifiers.diff added

code style - JLS specifies order of modifiers

Changed 6 years ago by shinigami

Attachment: stringbuilder.diff added

not-escaping local StringBuffer => StringBuilder

Changed 6 years ago by shinigami

Attachment: emptystring.diff added

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

comment:18 Changed 6 years ago by Don-vip

In 6085/josm:

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

comment:19 Changed 6 years ago by Don-vip

In 6086/josm:

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

comment:20 Changed 6 years ago by Don-vip

In 6087/josm:

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

comment:21 Changed 6 years ago by 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.

Changed 6 years ago by shinigami

Attachment: longs.diff added

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

comment:22 in reply to:  21 Changed 6 years ago by 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.

Changed 6 years ago by shinigami

Attachment: strcmp.diff added

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

comment:23 Changed 6 years ago by akks

In 6089/josm:

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

comment:24 Changed 6 years ago by akks

In 6090/josm:

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

comment:25 Changed 6 years ago by akks

Nice patches, thank you!

Changed 6 years ago by shinigami

Attachment: collsizetoempty.diff added

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

Changed 6 years ago by shinigami

Attachment: doublecmp.diff added

effective double comparison

Changed 6 years ago by shinigami

Attachment: bytestohexstring.diff added

byte[] to hex string change + small test

comment:26 Changed 6 years ago by stoecker

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

comment:27 Changed 6 years ago by akks

In 6093/josm:

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

comment:28 in reply to:  26 ; Changed 6 years ago by 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.

comment:29 in reply to:  28 Changed 6 years ago by 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 :-)

Last edited 6 years ago by stoecker (previous) (diff)

comment:30 Changed 6 years ago by 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;

Changed 6 years ago by shinigami

Attachment: datestocurrtime.diff added

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

Changed 6 years ago by shinigami

Attachment: whiletoforeach.diff added

while -> foreach

Changed 6 years ago by shinigami

Attachment: fortoforeach.diff added

for -> for each

Changed 6 years ago by shinigami

Attachment: instanceofnullcheck.diff added

removed useles null checks before instanceof

Changed 6 years ago by shinigami

Attachment: initsizes.diff added

set init size for lists when obvious

Changed 6 years ago by shinigami

Attachment: patterns.diff added

precompiled patterns

comment:31 Changed 6 years ago by 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.

comment:32 Changed 6 years ago by akks

I guess we should wait until stable release?

comment:33 Changed 6 years ago by Don-vip

In 6102/josm:

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

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

comment:34 Changed 6 years ago by 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

comment:35 Changed 6 years ago by Don-vip

In 6105/josm:

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

  • removed useles null checks before instanceof

comment:36 Changed 6 years ago by Don-vip

In 6106/josm:

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

  • set init size for lists when obvious

comment:37 in reply to:  32 Changed 6 years ago by 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.

Changed 6 years ago by shinigami

Attachment: collfix.diff added

problem in generic collection definition

Changed 6 years ago by shinigami

Attachment: styleiteration.diff added

useless iteration to do end of list ?

Changed 6 years ago by shinigami

Attachment: environment.diff added

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

Changed 6 years ago by shinigami

Attachment: warnings.diff added

compiler warnings

comment:38 Changed 6 years ago by 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?

comment:39 Changed 6 years ago by 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.

Changed 6 years ago by shinigami

Attachment: warnings.2.diff added

compiler warnings - against current version

comment:40 Changed 6 years ago by shinigami

oki, here is patch after update to head;)

comment:41 Changed 6 years ago by shinigami

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

comment:42 in reply to:  41 ; Changed 6 years ago by AlfonZ

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

Changed 6 years ago by shinigami

Attachment: entities.diff added

map of entities

comment:43 in reply to:  42 Changed 6 years ago by 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.

Changed 6 years ago by shinigami

Attachment: primitives.diff added

few optimizations around osm primitives

comment:44 Changed 6 years ago by shinigami

AbstractPrimitive

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

Prototype

  • optimized code creation

comment:45 Changed 6 years ago by Don-vip

In 6142/josm:

see #8902 - fix compilation warnings

Changed 6 years ago by shinigami

Attachment: quadbucketsindex.diff added

index count optimization

comment:46 Changed 6 years ago by 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%.

comment:47 Changed 6 years ago by stoecker

Hmm, it gets confusing.

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

comment:48 Changed 6 years ago by 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

comment:49 Changed 6 years ago by Don-vip

Summary: Small performance enhancements / coding style[Patches] Small performance enhancements / coding style

comment:50 Changed 6 years ago by 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

comment:51 Changed 6 years ago by Don-vip

Remaining ones:

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

(quadbucketsindex.diff done in #8986)

comment:52 Changed 6 years ago by 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?

comment:53 Changed 6 years ago by 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.

comment:54 Changed 5 years ago by bastiK

In 6821/josm:

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

primitives.diff: few optimizations around osm primitives

comment:55 Changed 5 years ago by bastiK

In 6822/josm:

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

stringbuilder.diff: StringBuffer => StringBuilder

comment:56 Changed 5 years ago by 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)

Last edited 5 years ago by stoecker (previous) (diff)

comment:57 in reply to:  51 Changed 5 years ago by bastiK

Resolution: fixed
Status: newclosed

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.