[Patch] JMapViewer task queue is single-threaded and can be dominated by stale tasks

[The111]

Older versions of JMapViewer used a custom implementation of a task queue. The current version uses a standard ExecutorService, which is in general a good idea. However, two major benefits of the old custom queue have been lost: multithreading, and cancellation of stale tasks. Without these two features, client performance is decreased, and server load is increased.

A simple experiment: fire up the Demo class from current HEAD code. Then do the following as quickly as possible:
1) zoom all the way out
2) zoom all the way in on the upper left corner
3) zoom all the way out
4) repeat steps 2 & 3 for all other corners
5) finally zoom all the way in on the center

I performed this experiment with instrumentation that counted every time a task finished and a tile was loaded. The actual input sequence described above takes only 10 seconds. Afterwards however I waited for over 10 minutes. In that time 2500+ tiles loaded, but I still could not see the portion of the map I was looking at, because all 2500 tiles that loaded were ones I was no longer interested in. It is very easy to clear the queue, in fact there is a method in place to do that and it is intentionally doing nothing now, which I seriously question. Once you fix this, you save 10 minutes of the user's time, and 2500 requests to the server (in my example at least, which is admittedly contrived, but the performance hit is real in every case even if less significant in normal use).

Also, the old custom task queue was multithreaded, however the new one is single-threaded which seems to me a very strange choice. I know this codebase has seen a lot of evolution since my last time here years ago (I've been busy since then with my ​real job), so perhaps there is some reason for this choice I'm unaware of... rate-limiting requests to server maybe?

Attached is a patch I would suggest we apply to fix both issues. I'm happy to do it myself, but I wanted to make sure the current active maintainers agree with my assessment, especially since I am, as ever, unfamiliar with JOSM which I know is the biggest use case for JMapViewer.

Thanks!

[simon04]

What about adding Executor jobDispatcher as an additional constructor parameter to permit using the best suited executor? For cancelOutstandingTasks we could check jobDispatcher instanceof ThreadPoolExecutor to cancel its tasks.

[The111]

Replying to simon04:

What about adding Executor jobDispatcher as an additional constructor parameter to permit using the best suited executor? For cancelOutstandingTasks we could check jobDispatcher instanceof ThreadPoolExecutor to cancel its tasks.

I don't see any reason to put that burden on the client unless there is a use case for different executors, and I don't see that either, but I am open to hearing what I'm missing. Currently there is only one client of OsmTileLoader in the JMapViewer anyway (TileController), and even that detail is abstracted from clients of JMapViewer, which is released as a standalone component. It would seem to make more sense to make an alternate implementation of TileLoader if different behavior was desired for some reason, but again if it was part of the JMapViewer package, then that choice of tile loader would need to be pushed up all the way to the JMapViewer user, which I don't see as being necessary, since all JMapViewer users should want better performance than it currently offers, I think.

But all of that is admittedly opinion-based, and more importantly irrelevant to my questions (or, at least until they are answered). The questions here are (1) why are we using a single-threaded tile loader and (2) why does it queue tasks forever, even after the user has moved to another part of the map? If there is no good reason for either, I am inclined to apply my patch to restore JMapViewer to the performance it had years ago.

[wiktorn]

Replying to The111:

But all of that is admittedly opinion-based, and more importantly irrelevant to my questions (or, at least until they are answered). The questions here are
(1) why are we using a single-threaded tile loader and

To prevent overloading of the server

(2) why does it queue tasks forever, even after the user has moved to another part of the map? If there is no good reason for either, I am inclined to apply my patch to restore JMapViewer to the performance it had years ago.

Indeed, I've missed that, when I was recently patching here.

As a quick fix in [o32007] I fixed the overloading part and switch to fixed, 3 threaded dispatcher, and added possibility for some other users to change the number of the threads.

Actually, within JOSM OsmTileLoader is not used at all, that's why this wasn't discovered till now.

[The111]

Replying to wiktorn:

To prevent overloading of the server

Ok, I suspected that might be why. Thanks for confirming.

As a quick fix in [o32007] I fixed the overloading part and switch to fixed, 3 threaded dispatcher, and added possibility for some other users to change the number of the threads.

Excellent, thanks. Three threads is still much better than one, and the overloading part was the bigger issue anyway. :-)

Actually, within JOSM OsmTileLoader is not used at all, that's why this wasn't discovered till now.

Ah, that makes more sense, and it explains what I found out yesterday when researching the SVN history, that the EDT was handling all IO for about half a year. I was wondering how such a serious issue went unnoticed for so long before you fixed it in [o31546] (good catch btw), but I guess maybe not many people are using OsmTileLoader. ​My app still uses a 2-year old binary of JMapViewer and I am totally fine with that, which is why I hadn't seen any of these recent changes either, until I noticed that a ​fork of my app was behaving strangely, which is what prompted my research.

Thanks for continuing to maintain JMapViewer!