Modify

Opened 2 years ago

Closed 2 years ago

#14796 closed enhancement (fixed)

[patch] Upgrade usability of JMapViewer

Reported by: chris.tipper@… Owned by: team
Priority: normal Milestone:
Component: JMapViewer Version: latest
Keywords: usability, performance Cc: wiktorn, bastiK, Klumbumbus

Description

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

Attachments (14)

JMapViewer.patch (15.4 KB) - added by chris.tipper@… 2 years ago.
JMapViewer patch based on svn trunk as of 12-05-2017 09:05 BST
bing_maps_logo_white_small.png (2.4 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_white_small.png 1st of batch
bing_maps_logo_gray_small.png (2.4 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_gray_small.png 2nd of batch
bing_maps_logo_white_medium_1.png (2.8 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_white_medium_1.png 3rd of batch
bing_maps_logo_gray_medium_1.png (2.9 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_gray_medium_2.png 4th of batch
bing_maps_logo_white_medium_2.png (4.5 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_white_medium_2.png 5th of batch
bing_maps_logo_gray_medium_2.png (4.6 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_gray_medium_2.png 5th of batch
bing_maps_logo_white_large.png (21.0 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_white_large.png 7th of batch
bing_maps_logo_gray_large.png (23.8 KB) - added by chris.tipper@… 2 years ago.
bing_maps_logo_gray_large.png 8th of batch
bing-attribution.patch (5.0 KB) - added by chris.tipper@… 2 years ago.
patch to load bing attribution asynchronously
bing-attribution-2.patch (5.3 KB) - added by chris.tipper@… 2 years ago.
bing-attribution-2.patch loads attribution asynchronously (tested briefly, initial load is erratic but continues successfully)
bing-attribution-3.patch (5.3 KB) - added by chris.tipper@… 2 years ago.
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.
JMapViewer-2.patch (6.3 KB) - added by chris.tipper@… 2 years ago.
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
JMapViewer-3.patch (6.4 KB) - added by chris.tipper@… 2 years ago.
revert OsmTileSource.java and add template for thunderforest transport map to demonstrate use of API key (cycle maps is no different in the end)

Download all attachments as: .zip

Change History (68)

Changed 2 years ago by chris.tipper@…

Attachment: JMapViewer.patch added

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

Changed 2 years ago by chris.tipper@…

bing_maps_logo_white_small.png 1st of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_gray_small.png 2nd of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_white_medium_1.png 3rd of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_gray_medium_2.png 4th of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_white_medium_2.png 5th of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_gray_medium_2.png 5th of batch

comment:1 Changed 2 years ago by stoecker

Cc: wiktorn bastiK added

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

I added the two experts for this code in CC.

Changed 2 years ago by chris.tipper@…

bing_maps_logo_white_large.png 7th of batch

Changed 2 years ago by chris.tipper@…

bing_maps_logo_gray_large.png 8th of batch

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

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

comment:4 Changed 2 years ago by Don-vip

Priority: minornormal

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

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

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

comment:8 in reply to:  5 Changed 2 years ago by 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.

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

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

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

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

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

comment:14 Changed 2 years ago by bastiK

JOSM requires Java 8 or later and so does JMapViewer.

Changed 2 years ago by chris.tipper@…

Attachment: bing-attribution.patch added

patch to load bing attribution asynchronously

comment:15 Changed 2 years ago by anonymous

Not extensively tested just wrote it right now.

comment:16 Changed 2 years ago by anonymous

Probably needs work, I apologise.

Changed 2 years ago by chris.tipper@…

Attachment: bing-attribution-2.patch added

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

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

Changed 2 years ago by chris.tipper@…

Attachment: bing-attribution-3.patch added

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.

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

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

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

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

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

Changed 2 years ago by chris.tipper@…

Attachment: JMapViewer-2.patch added

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

comment:23 in reply to:  22 Changed 2 years ago by 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?

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

comment:25 Changed 2 years ago by stoecker

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

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

comment:26 Changed 2 years ago by anonymous

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

comment:27 Changed 2 years ago by anonymous

"map"

Changed 2 years ago by chris.tipper@…

Attachment: JMapViewer-3.patch added

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

comment:28 in reply to:  27 Changed 2 years ago by stoecker

Replying to anonym:

"map"

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

comment:29 Changed 2 years ago by bastiK

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

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

comment:31 Changed 2 years ago by anonymous

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

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

comment:33 Changed 2 years ago by bastiK

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

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

comment:35 in reply to:  34 Changed 2 years ago by 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.

comment:36 Changed 2 years ago by bastiK

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

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

comment:38 Changed 2 years ago by bastiK

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

Last edited 2 years ago by bastiK (previous) (diff)

comment:39 in reply to:  37 ; Changed 2 years ago by 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.

Last edited 2 years ago by bastiK (previous) (diff)

comment:40 in reply to:  39 Changed 2 years ago by 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.

comment:41 Changed 2 years ago by Klumbumbus

Cc: Klumbumbus added

comment:42 Changed 2 years ago by 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".

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

    old new import org.openstreetmap.gui.jmapviewer. 
    2221 * @author Jan Peter Stotz
    2322 */
    2423public class OsmTileLoader implements TileLoader {
    25     private static final ThreadPoolExecutor jobDispatcher = (ThreadPoolExecutor) Executors.newFixedThreadPool(3);
     24    private static final ThreadPoolExecutor jobDispatcher = new ThreadPoolExecutor(8, 24, 30, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(2_000));
    2625
    2726    private final class OsmTileJob implements TileJob {
    2827        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

     
    2223 * @author Jan Peter Stotz
    2324 */
    2425public class OsmTileLoader implements TileLoader {
    25     private static final ThreadPoolExecutor jobDispatcher = (ThreadPoolExecutor) Executors.newFixedThreadPool(3);
     26    private static final ThreadPoolExecutor jobDispatcher = new ThreadPoolExecutor(8, 8, 30, TimeUnit.SECONDS, new LinkedBlockingDeque<>());
    2627
    2728    private final class OsmTileJob implements TileJob {
    2829        private final Tile tile;

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

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

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

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

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

comment:49 in reply to:  48 Changed 2 years ago by 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.

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

comment:51 in reply to:  50 Changed 2 years ago by 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).

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

comment:53 Changed 2 years ago by bastiK

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

comment:54 in reply to:  52 Changed 2 years ago by bastiK

Resolution: fixed
Status: newclosed

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!

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.