[patch] MapCSS rendering speed could be improved by reducing string/integer conversions.

[michael2402]

One Reason: Integer Preferences do a lot of Integer->String->Integer conversions

The current implementation of Preferences#getInteger always converts the given integer to a string to supply a default value, no matter if that string is ever required.

Removing this could increase MapCSS parsing time a lot.

MapCSS Rendering time can be seen e.g. when rendering an element for the first time. I did some testing with loading a big file and here are the timings for the first frame:
Phase 1: 74s (without profiler active)

I added an integer value cache to skip the integer/string conversions, this is the resulting render time:
Etwas verbessert (Patch im Git): Phase 1: 59,7s (without profiler active)

Either the MapCSS code should cache the preferences or the preferences should handle integers more efficiently.

I logged the preference access count, those were the results:

mappaint.node.tagged-size: 6855217
mappaint.node.unselected-size: 6855217
mappaint.node.connection-size: 6855217
mappaint.fillareas: 49
mappaint.fillalpha: 401978
mappaint.node.selected-size: 6855217

[michael2402]

This is the patch I tested with: ​https://github.com/michaelzangl/josm/commit/caa459520cbd7f961024850b82993022d7113789

[michael2402]

Please note that the patch probably breaks other things.

[bastiK]

In 8133/josm:

see #11227 - MapCSS: rendering slow because of frequent preferences lookup

[bastiK]

Nice catch! This is clearly a bug - it was not intended to look up these values for each node on the map. The immediate problem should be fixed by caching the numbers properly (in [8133]).

Regarding your patch: It is true, that getInteger does an unnecessary conversion to String and then back to int and it is a good idea to fix this. There are some issues with your patch though. Firstly, keep in mind, that preferences values change, e.g. they are set by the user in the advanced preferences GUI. This change would not propagate to your number cache, it would simply return the old value.
Secondly, this won't work:

@@ -1051,7 +1065,7 @@ public synchronized long getLong(String key, long def) {
     }
 
     public synchronized double getDouble(String key, double def) {
-        String v = get(key, Double.toString(def));
+        String v = get(key);
         if(null == v)
             return def;

get(String) returns the empty String and not null. (Use get(key, null).)

Finally, if you use cachedNumbers for nothing but integers, why is it stored as Long?

[anonymous]

Hi,

That patch was never intended to get integrated in JOSM. I just wanted to check how much the speed improvements from caching ints and longs were. Caching ints was impressive enough (and longs were not used much) so I did not touch longs. This was all just a quick testing hack.

The getDouble()-Change was untested, the cache never flushed and it probably did mess around with the way default values are handled, since those are not passed on to get().

Replying to bastiK:

Regarding your patch: It is true, that getInteger does an unnecessary conversion to String and then back to int and it is a good idea to fix this. There are some issues with your patch though. Firstly, keep in mind, that preferences values change, e.g. they are set by the user in the advanced preferences GUI. This change would not propagate to your number cache, it would simply return the old value.
Secondly, this won't work:

@@ -1051,7 +1065,7 @@ public synchronized long getLong(String key, long def) {
     }
 
     public synchronized double getDouble(String key, double def) {
-        String v = get(key, Double.toString(def));
+        String v = get(key);
         if(null == v)
             return def;

get(String) returns the empty String and not null. (Use get(key, null).)

Finally, if you use cachedNumbers for nothing but integers, why is it stored as Long?

[bastiK]

Replying to anonymous:

Hi,

That patch was never intended to get integrated in JOSM. I just wanted to check how much the speed improvements from caching ints and longs were. Caching ints was impressive enough (and longs were not used much) so I did not touch longs. This was all just a quick testing hack.

The getDouble()-Change was untested, the cache never flushed and it probably did mess around with the way default values are handled, since those are not passed on to get().

Alright, I get it. :)

The problem has been fixed as you suggested ("MapCSS code should cache the preferences"), so I'm closing this ticket. (There are useful ideas in the patch, but it can't be applied like this.)