[patch] Upgrade usability of JMapViewer

[chris.tipper@…]

Patch based on svn trunk as of 12-05-2017 09:05 BST

• OsmTileLoader uses tuned ThreadPoolExecutor, performance gain is dramatic on my developer machine
• cancelOutstandingTasks clears the task queue correctly
• BingAerialTileSource ensures it has loaded attribution, as required by the interface (the 0 timeout doesn't make sense)
• changes to OsmTileSource are optional and may be ignored (set http endpoint maybe not)
• CycleMap uses Thunderforest api with commented API_KEY
• TMSTileSource and ScanexTileSource latLonToXY round numerically, a workaround for the use of integer coordinates, it makes a difference to library users such as myself
• in Demo app move map uses standard drag gesture, previously it was incompatible with my trackpad

[chris.tipper@…]

JMapViewer patch based on svn trunk as of 12-05-2017 09:05 BST

[chris.tipper@…]

bing_maps_logo_white_small.png 1st of batch

[chris.tipper@…]

bing_maps_logo_gray_small.png 2nd of batch

[chris.tipper@…]

bing_maps_logo_white_medium_1.png 3rd of batch

[chris.tipper@…]

bing_maps_logo_gray_medium_2.png 4th of batch

[chris.tipper@…]

bing_maps_logo_white_medium_2.png 5th of batch

[chris.tipper@…]

bing_maps_logo_gray_medium_2.png 5th of batch

[stoecker]

One obvious issue on first view: Don't use "*" for imports.

I added the two experts for this code in CC.

[chris.tipper@…]

bing_maps_logo_white_large.png 7th of batch

[chris.tipper@…]

bing_maps_logo_gray_large.png 8th of batch

[anonymous]

Yes blame netbeans. By the way it is really hard to go back to generate a new patch I did a lot of other work to upscale tiles for retina macOS it's all mixed in I had to extract these changes.

[bastiK]

Thanks for your improvements to the JMapViewer component!

Replying to anonymous:

Yes blame netbeans.

Never had this problem with Netbeans.

By the way it is really hard to go back to generate a new patch I did a lot of other work to upscale tiles for retina macOS it's all mixed in I had to extract these changes.

It's not that hard: check out the repository into a new directory, apply the patch, done.

A short review of your changes:

• In DefaultMapController.java, the mouse buttons are switched. This is incompatible with current usage in JOSM. You could make it configurable, so it is up to library user.
• In BingAerialTileSource, the attribution loading is changed to a blocking call. This is not possible, as this code runs in event dispatch thread and the call fetches resources from the network, which may take several seconds.

[chris.tipper@…]

A comment on attribution loading on my machine tile loading will fail immediately without this change with no remediation. It is up to you but in my experience it does not take a few millis to load attribution and if you examine the tile loading code above this would need to be changed to handle this non-loading of attribution. This is a simpler remedy?

Of course reject any changes you do not see fit, I am just a member of the public submitting a useful patch I am not a pro developer, although the ThreadPool is the main thing that needed work.

[wiktorn]

As for the ThreadPool, I recommend setting RejectionExecutionHandler - I guess that ThreadPoolExecutor.DiscardOldestPolicy might be the best in this case.

Also - setting corePoolSize at 8 - is quite high compared to what typical browser would do. I understand that it might help, especially in high latency environments, but it may inflict high load on the servers though.

Can you explain, why removing one item by item is better than calling clear in one go? ​Documentation suggests that clear should work atomically.

[anonymous]

The javadoc for ThreadPoolExecutor recommends against using getQueue() for this purpose. getQueue() is 'intended primarily for debugging and monitoring.'

see ​http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html#remove(java.lang.Runnable)

[bastiK]

Replying to chris.tipper@…:

A comment on attribution loading on my machine tile loading will fail immediately without this change with no remediation.

The situation with Bing attribution loading isn't ideal in JOSM either. In your application you need to find a way to get notified when attribution is ready and then repaint.

It is up to you but in my experience it does not take a few millis to load attribution and if you examine the tile loading code above this would need to be changed to handle this non-loading of attribution. This is a simpler remedy?

Not everyone has stable and fast internet connection. Network download in EDT leads to freezes and is not acceptable for a widely used software.

Of course reject any changes you do not see fit, I am just a member of the public submitting a useful patch I am not a pro developer, although the ThreadPool is the main thing that needed work.

We have an interest to improve the JMapViewer code, so your participation is appreciated.

[anonymous]

I may be erring on this but I thought the tile loader was working in a thread pool and so there would not be concerns about blocking the event thread? A blocking call in the thread pool would eventually get removed from the queue in normal use? Also 8Mb/s internet here it is not what I would call a 'fast' internet connection!

[stoecker]

Replying to anonym:

Also 8Mb/s internet here it is not what I would call a 'fast' internet connection!

If you had a 256kBit/s you would.

[chris.tipper@…]

A related point I notice https endpoints put a certain strain on connection to Osm tile servers. Security I know but the difference compared to Bing is noticeable (presumably operating on Azure). I am sure I am not nearly an expert on security but we are loading map tiles this seems to me to be a trade-off you could consider making in the name of saving connection resources.

[bastiK]

Replying to anonymous:

I may be erring on this but I thought the tile loader was working in a thread pool and so there would not be concerns about blocking the event thread?

It is called from painting code which runs in EDT. You can easily check by setting a break point or printing the Thread name to standard output.

A related point I notice https endpoints put a certain strain on connection to Osm tile servers. Security I know but the difference compared to Bing is noticeable (presumably operating on Azure). I am sure I am not nearly an expert on security but we are loading map tiles this seems to me to be a trade-off you could consider making in the name of saving connection resources.

This is a matter of philosophy, but we try to use https whenever possible. Plain http is an anachronism, connections should be encrypted by default. If it is a strain on the technology, then the technology needs to improve (and it does).

[chris.tipper@…]

Yes I see that now. I would be tempted to reimplement as CompletableFuture with a callback. It's really not worth staying with Java 7 for this sort of threading work in my experience. I'm maintaining compatibility for your sake, but I think Oracle recommend projects move Java 8, I saw this case of an unpatched rendering bugs in Java 7 ​https://bugs.openjdk.java.net/browse/JDK-8029253

[bastiK]

JOSM requires Java 8 or later and so does JMapViewer.

[chris.tipper@…]

patch to load bing attribution asynchronously

[anonymous]

Not extensively tested just wrote it right now.

[anonymous]

Probably needs work, I apologise.

[chris.tipper@…]

bing-attribution-2.patch loads attribution asynchronously (tested briefly, initial load is erratic but continues successfully)

[anonymous]

I'm definitely noticing performance regression with this patch, I would press the blocking future. It seems to be extracting the tile url from the same document, but I may be mistaken.

[chris.tipper@…]

Some minor changes, performance is good if you don't call getAttribution() inside getTileUrl(), however the problem with initial load discourages me from recommending this fix.

[anonymous]

I ran some ad hoc testing with the get timeout I estimate on my connection the round trip delay at the start of the Bing maps lookup is ~1000ms It's not good, but I have been using the library for quite a while and it doesn't cause problems with the EDT in my experience. Also 256Kbps? Do people still use ISDN? Amazing.

[bastiK]

[...] however the problem with initial load discourages me from recommending this fix.

Please send patches that you do recommend and describe shortly what it does.

[anonymous]

I do recommend patch #14796 I spent considerable effort distilling small meaningful changes in that one patch. The patches attached to the thread are for example only and were only a suggestion as to how you may modernize the library. My other work on upscaling tiles for retina macOS can wait another day, as frankly I am surprised by the reception I have received. I thought that there was absolutely nothing to object to in my submission it was a deliberately low key first approach. Good luck with your future projects.

[bastiK]

Replying to anonymous:

The patches attached to the thread are for example only and were only a suggestion as to how you may modernize the library.

Thanks, this wasn't clear to me.

I thought that there was absolutely nothing to object to in my submission [...].

You must have realized by now that this is not the case. Sorry if the overall reaction came across a bit harsh. We don't want to disparage your work or attack you personally. Quite the opposite: We value the contribution and like to integrate your changes into the repository. To that end you get premium support in fixing the problems preventing that.

Good luck with your future projects.

Likewise.

[chris.tipper@…]

I understand the need to support JOSM but can I say on the Bing Aerial maps issue as it stands the end-user experience is pretty bad. I can understand you wanting to fix it as opposed to accepting my change, but can I say as an app developer that it is too easy to obsess over the EDT. I know all about it, I have quite some multi-threading code of my own, it's not as big a problem as it seems in this case. At least put in a decent time out, it's the app developer that gets blamed when maps load in such an ugly and haphazard way.

I didn't take it personally I'm alright with the back and forth. I am just surprised that's all.

[chris.tipper@…]

revised patch based off today's svn repo, removed the changes objected, inserted a 1500ms timeout for Bing based on local machine perf, eliminated redundant whitespace

[bastiK]

Replying to chris.tipper@…:

I understand the need to support JOSM but can I say on the Bing Aerial maps issue as it stands the end-user experience is pretty bad. I can understand you wanting to fix it as opposed to accepting my change, but can I say as an app developer that it is too easy to obsess over the EDT.

Hmm, maybe.

I know all about it, I have quite some multi-threading code of my own, it's not as big a problem as it seems in this case. At least put in a decent time out, it's the app developer that gets blamed when maps load in such an ugly and haphazard way.

Why not wait for attribution loading in the thread that downloads the tile? You'd have 2 downloads in succession instead of one, but all in the background. (This is just a rough idea, I haven't looked at it from implementation point of view.)

I didn't take it personally I'm alright with the back and forth. I am just surprised that's all.

Sounded pretty upset for a British person.

revised patch based off today's svn repo, removed the changes objected, inserted a 1500ms timeout for Bing based on local machine perf, eliminated redundant whitespace

Have you seen wiktorn's remarks? Is it supposed to be API_KEY = "API_KEY"; or do you have to register a key?

[anonymous]

Yes register a key. I don't know much about this project but I know you are working with a code base dating back ten years and I have only gained familiarity in the last fortnight. If your team want to push ahead and revamp this class (BingAerialTileSource) that would make sense. I have until recently been using a much older version which is why this is unfamiliar to me.

[stoecker]

I'd drop that Key stuff for now. Maps has no key as well and works.

[anonymous]

You're right I found thunderforest looking for all the old mail types. If you need extensibility this is useful template code.

[anonymous]

"map"

[chris.tipper@…]

revert OsmTileSource.java and add template for thunderforest transport map to demonstrate use of API key (cycle maps is no different in the end)

[stoecker]

Replying to anonym:

"map"

You can change a comment also after you sent it. Mouseover over top right of the comment field :-)

[bastiK]

In [o33313]: see ​#14796 - various improvements for JMapViewer (patch by Chris Tipper)

[bastiK]

I have not committed the changes to OsmTileLoader.java as the discussion seems to be still in progress. Same for the Bing attribution change. Yes, it is broken, but we should find a proper fix.

[anonymous]

Thanks sounds reasonable. Sorry to be argumentative unfamiliar around here.

[Don-vip]

Replying to bastiK:

In [o33313]: see ​#14796 - various improvements for JMapViewer (patch by Chris Tipper)

API_KEY should either be defined/used or removed. See Sonar :)

[bastiK]

In [o33314]: see ​#14796 - make use of private API_KEY field

[stoecker]

This will issue requests with API_KEY as default. My suggestion would be to make the value empty and comment to fill it and only add it if not empty.

BTW: Now Transport map has been added, but the cycle map fixes are gone?

[bastiK]

Replying to stoecker:

This will issue requests with API_KEY as default. My suggestion would be to make the value empty and comment to fill it and only add it if not empty.

It is a copy & paste template and is not supposed to work without a proper API key (as stated in the Javadoc). Feel free to change, but we can just remove this class if it's too much trouble.

BTW: Now Transport map has been added, but the cycle map fixes are gone?

Not sure we should switch to thunderforest.com in JOSM. We may or may not be hitting the "Hobby Project" limit of 150,000 tiles per month, but better stick to what works.

[bastiK]

Okay, we are already using thunderforest, but without API key, which is has become mandatory: ​http://thunderforest.com/terms/.

[stoecker]

I contacted Thunderforest about the API keys for our Map entries and also the JMapViewer.

The old cycle URL very likely is only a forwarder to thunderforest.com.

[bastiK]

In [o33320] - see #14796 - make API key an abstract getter

[bastiK]

Replying to stoecker:

I contacted Thunderforest about the API keys for our Map entries and also the JMapViewer.

You just need to sign up, no?

edit: As JMapViewer is a library, probably every app using it should sign up separately.

[stoecker]

Replying to bastiK:

You just need to sign up, no?

Well. That's not so easy. Think about our imagery sync process which has issues with different ID's for the editors and others will simply copy the API key instead requesting own and ...

edit: As JMapViewer is a library, probably every app using it should sign up separately.

Exactly.

I simply asked about their conditions in our case. We will see what they answer.

[stoecker]

Some news:

• I dropped CycleMap from JOSM SlippyMap, as that really makes no longer sense nowadays.
• We have an JOSM API-Key which I added to our maps
• I suggest to drop the class OsmTileSource and instead make individual classes from it. It makes no sense to group these
• The CylcleMap should either be changed to Thunderforest as well or dropped, as the license conditions for this old URL is totally unclear. When dropped another example without API key can be added, e.g. "Stamen Terrain".

[bastiK]

As far as I'm aware, the last remaining change to be applied is the improvement to the ThreadPoolExecutor. I agree with wiktorn, that there are issues with the patch as is:

src/org/openstreetmap/gui/jmapviewer/OsmTileLoader.java

@@ -22 +21 @@
  * @author Jan Peter Stotz
  */
 public class OsmTileLoader implements TileLoader {
-    private static final ThreadPoolExecutor jobDispatcher = (ThreadPoolExecutor) Executors.newFixedThreadPool(3);
+    private static final ThreadPoolExecutor jobDispatcher = new ThreadPoolExecutor(8, 24, 30, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(2_000));
 
     private final class OsmTileJob implements TileJob {
         private final Tile tile;

As already mentioned, the number of concurrent connections per host should be about 8, not 24. In addition, setting the core pool size to 8 and the maximum pool size to 24, along with a fixed capacity queue of 2000 entries creates a rather peculiar execution strategy. I suggest a simpler version (fixed pool size with LIFO):

src/org/openstreetmap/gui/jmapviewer/OsmTileLoader.java

@@ -22 +23 @@
  * @author Jan Peter Stotz
  */
 public class OsmTileLoader implements TileLoader {
-    private static final ThreadPoolExecutor jobDispatcher = (ThreadPoolExecutor) Executors.newFixedThreadPool(3);
+    private static final ThreadPoolExecutor jobDispatcher = new ThreadPoolExecutor(8, 8, 30, TimeUnit.SECONDS, new LinkedBlockingDeque<>());
 
     private final class OsmTileJob implements TileJob {
         private final Tile tile;

[chris.tipper@…]

May I quote the header from the ThreadpoolExecutor API page <​http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadPoolExecutor.html>

Unbounded queues. Using an unbounded queue (for example a LinkedBlockingQueue without a predefined capacity) will cause new tasks to wait in the queue when all corePoolSize threads are busy. Thus, no more than corePoolSize threads will ever be created. (And the value of the maximumPoolSize therefore doesn't have any effect.) This may be appropriate when each task is completely independent of others, so tasks cannot affect each others execution; for example, in a web page server. While this style of queuing can be useful in smoothing out transient bursts of requests, it admits the possibility of unbounded work queue growth when commands continue to arrive on average faster than they can be processed.

[bastiK]

Yes, this is how Executors.newFixedThreadPool works and it's the intended behavior (don't drop any download requests, just delay them if needed). For reference, see also TMSCachedTileLoader.getNewThreadPoolExecutor​ for the well-tested JOSM-implementation.

[anonymous]

Yes I've seen the source, why don't you go back to the original and ignore the patch, no need to set up your own threadpool if you only want default behaviour, I was dissatisfied with the performance in macOS. I did tune it for greater throughput for my application. maxPoolSize is a limit as far as I understand the corePoolSize is more important.

[bastiK]

You are right, the max pool size only comes into play, when the requests pile up such that the capacity of the queue is exceeded. This means that under normal circumstances, my suggested patch behaves similarly to your's (including better performance). It just cuts the frills that makes it wonky in the exceptional case of many requests (randomly dropped requests and increase to 24 concurrent downloads).

Another change I suggested is to move from a FIFO to LIFO, so more recent requests are prioritized.

[anonymous]

Isn't what you've suggested just ThreadPoolExecutor) Executors.newFixedThreadPool(8); I've no other objection but I'm not sure if you are asking me to submit another patch, it seems easy to just change it yourself.

[bastiK]

Replying to anonymous:

Isn't what you've suggested just ThreadPoolExecutor) Executors.newFixedThreadPool(8);

Nope, Executors.newFixedThreadPool uses LinkedBlockingQueue instead of LinkedBlockingDeque.

I've no other objection but I'm not sure if you are asking me to submit another patch, it seems easy to just change it yourself.

There's no need to submit another patch for this. I've just put it here to give everyone a chance to comment. If there are no objections, I'll commit the change and close the ticket.

[chris.tipper@…]

I think macos performance characteristics are quite different from your platform I've just conducted some A/B testing on my development machine (macOS 10.12.5) and my changes make a difference. I don't quite understand the reason for not having a larger maxPoolSize, though the ThreadPoolExecutor javadoc does have this comment:

Bounded queues. A bounded queue (for example, an ArrayBlockingQueue) helps prevent resource exhaustion when used with finite maximumPoolSizes, but can be more difficult to tune and control. Queue sizes and maximum pool sizes may be traded off for each other: Using large queues and small pools minimizes CPU usage, OS resources, and context-switching overhead, but can lead to artificially low throughput.

My feeling is that this is platform dependent and that if non of your users are on macOS then go ahead with your amendments. I would urge you to investigate further the LinkedBlockingDeque as it seems like a mistake, it seems to produce worse results other things being equal.

[bastiK]

Replying to chris.tipper@…:

I think macos performance characteristics are quite different from your platform I've just conducted some A/B testing on my development machine (macOS 10.12.5) and my changes make a difference.

This is interesting, you are welcome to share your benchmark setup.

I don't quite understand the reason for not having a larger maxPoolSize, though the ThreadPoolExecutor javadoc does have this comment:

Bounded queues. A bounded queue (for example, an ArrayBlockingQueue) helps prevent resource exhaustion when used with finite maximumPoolSizes, but can be more difficult to tune and control. Queue sizes and maximum pool sizes may be traded off for each other: Using large queues and small pools minimizes CPU usage, OS resources, and context-switching overhead, but can lead to artificially low throughput.

My feeling is that this is platform dependent

Why would it be? The bottleneck is the network and not the CPU.

and that if non of your users are on macOS then go ahead with your amendments.

We have a normal fraction of macOS users.

I would urge you to investigate further the LinkedBlockingDeque as it seems like a mistake, it seems to produce worse results other things being equal.

The parameters cannot be tuned freely, there are certain reasons and limitations:

• The number of concurrent downloads per host is limited to 8. This means both core pool size and max pool size must be 8 or less. Increasing those numbers may speed up the download, but it is policy to keep it at 8. A much higher number would be abusive server usage (borderline DDoS).
• If all requests should be processed, then the size of the queue must be unlimited.
• Lastly, it makes more sense to download tiles first that have been requested more recently, because this is supposedly what the user wants to see right now. Therefore a LIFO (LinkedBlockingDeque) should be preferred over a FIFO (LinkedBlockingQueue).

[anonymous]

I think we must agree to difference on this. As an app developer I care mostly about user expectations and corePoolSize seems a perfectly fine limit, maxPoolSize is headroom for initial load from my point of view. However I clearly do not have thousands of users so maybe my needs are less stringent. The user actually wants what he is looking at to load first and your other code already caters for this by loading tiles in an expanding spiral, when peripheral tiles load first this is just distracting from a user perspective. Also the only real way I could distinguish the various different configurations was by turning on https transport so network is the decisive role as you say. I don't know who your hosting provider is but for some reason java https transport is the real bottleneck and I can't say why, comparison with Bing maps is not flattering presumably hosted in Azure.

[bastiK]

In [o33431]: see ​#14796 - increase the number of concurrent downloads from 3 to 8 (based on patch by Chris Tipper)

[bastiK]

Replying to anonymous:

I think we must agree to difference on this. As an app developer I care mostly about user expectations and corePoolSize seems a perfectly fine limit, maxPoolSize is headroom for initial load from my point of view.

What I get from the JavaDoc is that the maxPoolSize value will be ignored, unless the queue is beyond capacity.

However I clearly do not have thousands of users so maybe my needs are less stringent. The user actually wants what he is looking at to load first and your other code already caters for this by loading tiles in an expanding spiral, when peripheral tiles load first this is just distracting from a user perspective.

Good point. Considering that the queue is cleared, whenever the map is zoomed or moved, there isn't really a reason to be extra smart with the order of requests. (There is also no way a queue with a capacity of 2000 elements would ever get saturated.) So I went with the simplest solution.

Anyway, thanks for your contributions!