Modify

Opened 6 years ago

Closed 6 years ago

#11227 closed enhancement (fixed)

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

Reported by: michael2402 Owned by: team
Priority: normal Milestone: 15.03
Component: Core Version:
Keywords: Preferences Cc: Klumbumbus

Description

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

Attachments (0)

Change History (9)

comment:2 Changed 6 years ago by Klumbumbus

Cc: Klumbumbus added
Priority: minornormal

comment:3 Changed 6 years ago by Klumbumbus

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

comment:4 Changed 6 years ago by michael2402

Please note that the patch probably breaks other things.

comment:5 Changed 6 years ago by bastiK

In 8133/josm:

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

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

comment:7 Changed 6 years ago by Klumbumbus

Milestone: 15.03

comment:8 in reply to:  6 ; Changed 6 years ago by 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?

comment:9 in reply to:  8 Changed 6 years ago by bastiK

Resolution: fixed
Status: newclosed

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

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.