Limit size of TMS cache

[wiktorn]

Attached patched introduces Apache Commons Java Caching System (​http://commons.apache.org/proper/commons-jcs) as a backend for TMS cache. I've considered also EhCache, but EhCache has a lot more dependencies and I found hard, to trim it down to reasonable size.

Adding this, raises josm.jar by about 500kb, and josm-optimized.jar by 300kb.

Apache JCS has a limitation, that it can only limit the number of entries, not its size. I've introduced static fields with preconfigured sizes:

• 1000 (~20mb) of keys in memory
• 100000 (2000mb) of keys on disk

These can probably can get promoted to properties or even preferences.

I suspect that there is also another layer of tiles caching, which can be removed now.

I tried also switch WMS Cache to this solution, but I don't understand the solution there:

• why we are using Lists for elements instead of HashMaps
• what is the concurency model here (why not use ConcurrentHashMap or anything similar)

I'd assume different cache store (cache region) for WMS, which then would occupy separate diskspace/memory.

[bastiK]

Replying to wiktorn:

Attached patched introduces Apache Commons Java Caching System (​http://commons.apache.org/proper/commons-jcs) as a backend for TMS cache. I've considered also EhCache, but EhCache has a lot more dependencies and I found hard, to trim it down to reasonable size.

Adding this, raises josm.jar by about 500kb, and josm-optimized.jar by 300kb.

In my tests, I get an increase of 226 KiB. After stripping the imports of org.apache.commons.jcs.admin.JCSAdminBean and org.apache.commons.jcs.auxiliary.remote.behavior.IRemoteCacheConstants from the class CompositeCacheManager, this gets down to 197 KiB. The remaining classes look useful on first sight, so this should be about the minimum.

(To get these values, I run javac -cp .:src -d build src/org/openstreetmap/josm/gui/MainApplication.java, then jar c build > classes.jar.)

Apache JCS has a limitation, that it can only limit the number of entries, not its size.

Not optimal, but I think we can live with that.

I've introduced static fields with preconfigured sizes:

• 1000 (~20mb) of keys in memory
• 100000 (2000mb) of keys on disk

These can probably can get promoted to properties or even preferences.

I suspect that there is also another layer of tiles caching, which can be removed now.

Could you explain?

I tried also switch WMS Cache to this solution, but I don't understand the solution there:

• why we are using Lists for elements instead of HashMaps

Do you mean HashSet instead of List in ProjectionEntries.entries? I don't see a reason. If you think it is necessary, we can change it to HashSet now and see if if we get any bugreports as a result. :)

• what is the concurency model here (why not use ConcurrentHashMap or anything similar)

No idea. Either it isn't used from multiple threads or it is extremely unlikely to have concurrent modifications for some reason. Anyway, it shouldn't be a problem to add synchronization, if needed.

I'd assume different cache store (cache region) for WMS, which then would occupy separate diskspace/memory.

Alright.

Could you explain a bit, how the jcs cache works? Which entries will be removed, when the cache is overflowing?

How does the migration work, when we need to add more properties to the cache element?

@team: Please comment, if you have an opinion. :)

[wiktorn]

Replying to bastiK:

Replying to wiktorn:

These can probably can get promoted to properties or even preferences.

I suspect that there is also another layer of tiles caching, which can be removed now.

Could you explain?

I haven't yet look for it, but if I pan the map around, and go back to initial place, I don't even see hitting JCS memory cache, even though tiles are not downloaded. So I suspect that there is some other caching mechanism here.

I tried also switch WMS Cache to this solution, but I don't understand the solution there:

• why we are using Lists for elements instead of HashMaps

Do you mean HashSet instead of List in ProjectionEntries.entries? I don't see a reason. If you think it is necessary, we can change it to HashSet now and see if if we get any bugreports as a result. :)

No - I mean HashMaps - for now, we are always traversing half of list, to find an entry, when we could just ask for proper one by key. Check private CacheEntry findEntry(...)

• what is the concurency model here (why not use ConcurrentHashMap or anything similar)

No idea. Either it isn't used from multiple threads or it is extremely unlikely to have concurrent modifications for some reason. Anyway, it shouldn't be a problem to add synchronization, if needed.

That's the problem I see now, a lot of synchronized methods around, but I don't see a reason. I hope that I will manage to move synchronization somewhere lower.

I'd assume different cache store (cache region) for WMS, which then would occupy separate diskspace/memory.

Alright.

Could you explain a bit, how the jcs cache works? Which entries will be removed, when the cache is overflowing?

For memory, JCS uses LRU algorithm (element access moves the element to the front of queue, we are discarding elements from the end of queue) and this can be easily changed. For disk, JCS always uses LRU.

How does the migration work, when we need to add more properties to the cache element?

Because we will bump the serialVersionUID, deserialization will fail, so we will discard all cache in such case. But in gentle manner. When I was playing with JCS parameters that made files incompatibile, JCS complained in the logs, but just recreated files, so it's quite user friendly.

We will have to address user migration to new scheme (and somehow delete old cache files) though.

[bastiK]

Replying to wiktorn:

Replying to bastiK:

Replying to wiktorn:

These can probably can get promoted to properties or even preferences.

I suspect that there is also another layer of tiles caching, which can be removed now.

Could you explain?

I haven't yet look for it, but if I pan the map around, and go back to initial place, I don't even see hitting JCS memory cache, even though tiles are not downloaded. So I suspect that there is some other caching mechanism here.

Yes, TMSLayer.tileCache is still used.

I tried also switch WMS Cache to this solution, but I don't understand the solution there:

• why we are using Lists for elements instead of HashMaps

Do you mean HashSet instead of List in ProjectionEntries.entries? I don't see a reason. If you think it is necessary, we can change it to HashSet now and see if if we get any bugreports as a result. :)

No - I mean HashMaps - for now, we are always traversing half of list, to find an entry, when we could just ask for proper one by key. Check private CacheEntry findEntry(...)

Okay, a HashMap would be better, probably. But with WMS you have the problem, that there isn't always an exact match, so you still have to iterate the entire collection (like in getPartialMatch). I think this is even more common than a direct hit, so the advantage of a HashMap would be restricted to one special case.

• what is the concurency model here (why not use ConcurrentHashMap or anything similar)

No idea. Either it isn't used from multiple threads or it is extremely unlikely to have concurrent modifications for some reason. Anyway, it shouldn't be a problem to add synchronization, if needed.

That's the problem I see now, a lot of synchronized methods around, but I don't see a reason. I hope that I will manage to move synchronization somewhere lower.

Without investigating in detail, WMSGrabber probably writes to the cache in a background thread and most operations like getting tiles for painting is initiated from EDT. So it seems normal to synchronize all methods that access the underlying data storage.

Could you explain a bit, how the jcs cache works? Which entries will be removed, when the cache is overflowing?

For memory, JCS uses LRU algorithm (element access moves the element to the front of queue, we are discarding elements from the end of queue) and this can be easily changed. For disk, JCS always uses LRU.

Okay, sounds good.

How does the migration work, when we need to add more properties to the cache element?

Because we will bump the serialVersionUID, deserialization will fail, so we will discard all cache in such case. But in gentle manner. When I was playing with JCS parameters that made files incompatibile, JCS complained in the logs, but just recreated files, so it's quite user friendly.

We will have to address user migration to new scheme (and somehow delete old cache files) though.

Then what about putting all our parameters in a HashMap<String,String>? This way we can add or remove parameters at will, without touching the class definition.

Another thing: Is there a reason, you set the element attribute MaxLife to one month? This should correspond to the value ABSOLUTE_EXPIRE_TIME_LIMIT, which is currently unlimited.

It would be nice to have a graphical option in the preferences for the cache size (disk cache) and maybe for the MaxLife value. But this doesn't need to part of the current patch, it can be added later.

[wiktorn]

Replying to bastiK:

Replying to wiktorn:

I haven't yet look for it, but if I pan the map around, and go back to initial place, I don't even see hitting JCS memory cache, even though tiles are not downloaded. So I suspect that there is some other caching mechanism here.

Yes, TMSLayer.tileCache is still used.

Ok, so I'll work further, to replace TMSLayer.tileCache with JCS. I plan to do something similar to org.apache.commons.jcs.utils.accessJCSWorker - so there will be class that is responsible for whole task:

• get from cache, if it's there (disk or memory)
• if not, create a job for worker thread to fetch data
• handle all specific cases for HTTP caching

And I hope to cover also WMS Cache by the way.

No - I mean HashMaps - for now, we are always traversing half of list, to find an entry, when we could just ask for proper one by key. Check private CacheEntry findEntry(...)

Okay, a HashMap would be better, probably. But with WMS you have the problem, that there isn't always an exact match, so you still have to iterate the entire collection (like in getPartialMatch). I think this is even more common than a direct hit, so the advantage of a HashMap would be restricted to one special case.

I did simple check on Geoportal (ortofoto) layer in Poland. For 107 calls to getPartialMatch, I counted 1600 calls to findEntry, so optimizing findEntry case may have some merit.

Then what about putting all our parameters in a HashMap<String,String>? This way we can add or remove parameters at will, without touching the class definition.

Good idea, I'll correct that.

Another thing: Is there a reason, you set the element attribute MaxLife to one month? This should correspond to the value ABSOLUTE_EXPIRE_TIME_LIMIT, which is currently unlimited.

I stand corrected.

It would be nice to have a graphical option in the preferences for the cache size (disk cache) and maybe for the MaxLife value. But this doesn't need to part of the current patch, it can be added later.

When the patch will be polished, I plan to add tweakings to the GUI:

• number of elements in TMS disk cache
• number of elements in TMS memory cache
• number of elements in WMS disk cache
• number of elements in WMS memory cache

I would like to keep them separated because I'll use different classes as the cache key. The drawback is that then space is not shared between this caches.

[bastiK]

I found two more use cases for JCS. :)

First is the thumbnail cache for geotagged images. Second would be for the downloaded images of the upcoming Mapillary plugin.

[wiktorn]

Great.

I'm reworking the patch, so there will be a generic API, that will accept URL and give more-less web-browser-class caching, and above it, there will be wrapper layer for getting tiles (WMS or TMS).

I guess this could also be used in presets downloader and probably many more uses.

[bastiK]

Actually I prefer to have the cached content as regular files. Then you can open it in an editor, inspect, copy, change and remove it. In case of TMS/WMS, the size limitation feature for the disk cache (in my opinion) outweighs this little disadvantage of JCS.

But I see no reason to use it for things like presets, where we do not have the problem, that the cache grows without bounds.

[wiktorn]

The only reason I thought it would be useful, is to reuse the logic to manage cache expiration, but I see the point in simplicity of verification of the file.

For now, I identified also some nasty bug within JCS:
​https://issues.apache.org/jira/browse/JCS-144

It will result in hanged JOSM, as I decided not to isolate cache get's into a separate threads, to avoid intra-thread communication. Thus the main AWT thread stucks.

I hope this will get fixed, or we can prepare same fix, as there is for IndexedDiskCache, which is to release the lock before calling reset().

I chose BlockDiskCache over IndexedDiskCache because of this statement:

The Block Disk Cache has advantages over the normal indexed model for regions where the size of the items varies. Since all the blocks are the same size, the recycle bin is very simple. It is just a list of block numbers. Also, the Block Disk Cache will never need to be optimized. Once the maximum number of keys is reached, blocks will be reused.

​http://commons.apache.org/proper/commons-jcs/BlockDiskCache.html

For now as a workaround we can use IndexedDiskCache.

Apart from implementing JCS cache, I've also pushed down synchronized methods, so locks are fair grained.

I haven't yet touched WMS cache, as there I'd like first to move away from double to BigDecimal or fix floating-point arithmetics, and then introduce JCS.

[wiktorn]

patch generated by svn diff

[bastiK]

I think you forgot the changes to the code in the external repository org/openstreetmap/gui/jmapviewer (doesn't compile).

It seems, you are duplicating quite a lot of code. I'd prefer if you refactor the jmapviewer classes, so they can be used for JCS. We can move the classes to a more appropriate package (inside the jmapviewer repo), if you like.

[wiktorn]

I guess svn diff didn't take into account external repo. I'll fix that.

Can you explain, why do we keep external repo with jmapviewer? Within my patch I moved most of the work outside of jmapviewer. The only class that I duplicate is JobDispatcher, as I need to change the signature. But I can adapt that too.

[bastiK]

Replying to wiktorn:

I chose BlockDiskCache over IndexedDiskCache because of this statement:

The Block Disk Cache has advantages over the normal indexed model for regions where the size of the items varies. Since all the blocks are the same size, the recycle bin is very simple. It is just a list of block numbers. Also, the Block Disk Cache will never need to be optimized. Once the maximum number of keys is reached, blocks will be reused.

​http://commons.apache.org/proper/commons-jcs/BlockDiskCache.html

I don't understand the advantage of BlockDiskCache, but I suppose it doesn't matter much which one we choose.

I haven't yet touched WMS cache, as there I'd like first to move away from double to BigDecimal or fix floating-point arithmetics, and then introduce JCS.

Could you explain the problems you see with floating-point arithmetics?

Replying to osm@…:

Can you explain, why do we keep external repo with jmapviewer? Within my patch I moved most of the work outside of jmapviewer.

JMapViewer was created as an independent Java library for displaying TMS slippy maps. It was always an integral part of JOSM, but apparently it is used by other projects as well. I know it is inconvenient, but so far we were able to keep the code separate.

The only class that I duplicate is JobDispatcher, as I need to change the signature. But I can adapt that too.

I may have exaggerated.

[wiktorn]

Replying to bastiK:

I don't understand the advantage of BlockDiskCache, but I suppose it doesn't matter much which one we choose.

My guess is, that it needs less bookkeeping as opposed to IndexedDiskCache, which may need to optimize storage file from time to time. But it may not play a big role in our use case, so for now, we can switch to IndexedDiskCache.

I haven't yet touched WMS cache, as there I'd like first to move away from double to BigDecimal or fix floating-point arithmetics, and then introduce JCS.

Could you explain the problems you see with floating-point arithmetics?

	    private CacheEntry findEntry(ProjectionEntries projectionEntries, double pixelPerDegree, double east, double north) {
	        for (CacheEntry entry: projectionEntries.entries) {
	            if (entry.pixelPerDegree == pixelPerDegree && entry.east == east && entry.north == north)
	                return entry;
	        }
	        return null;
	    }

Here I would see something like Math.abs(entry.pixelPerDegree - pixelPerDegree) < EPSILON or Math.abs(entry.pixelPerDegree - pixelPerDegree) / pixelPerDegree < EPSILON. Or just use BigDecimals... More on this for example here:
​http://stackoverflow.com/questions/356807/java-double-comparison-epsilon

Replying to osm@…:

Can you explain, why do we keep external repo with jmapviewer? Within my patch I moved most of the work outside of jmapviewer.

JMapViewer was created as an independent Java library for displaying TMS slippy maps. It was always an integral part of JOSM, but apparently it is used by other projects as well. I know it is inconvenient, but so far we were able to keep the code separate.

The question that needs answer then is, whether we like to jmapviewer be dependent on JCS Cache (meaning - additional dependencies for external project).

If it is the case, and there is no problem, in having general URL cache within jmapviewer code base, I'll move that to jmapviewer tree.

[bastiK]

Replying to osm@…:

	    private CacheEntry findEntry(ProjectionEntries projectionEntries, double pixelPerDegree, double east, double north) {
	        for (CacheEntry entry: projectionEntries.entries) {
	            if (entry.pixelPerDegree == pixelPerDegree && entry.east == east && entry.north == north)
	                return entry;
	        }
	        return null;
	    }

Here I would see something like Math.abs(entry.pixelPerDegree - pixelPerDegree) < EPSILON or Math.abs(entry.pixelPerDegree - pixelPerDegree) / pixelPerDegree < EPSILON. Or just use BigDecimals... More on this for example here:
​http://stackoverflow.com/questions/356807/java-double-comparison-epsilon

Why? I still don't see what is wrong with the code or why it would be an improvement to use BigDecimals.

The question that needs answer then is, whether we like to jmapviewer be dependent on JCS Cache (meaning - additional dependencies for external project).

If it is the case, and there is no problem, in having general URL cache within jmapviewer code base, I'll move that to jmapviewer tree.

It would be a nice to have a modular structure, where you can plug in your own cache implementation. Then I see no technical reason to have JCS dependencies in jmapviewer.

On the other hand, why not. (As long as you can still compile jmapviewer without the JCS module.)

[wiktorn]

Replying to bastiK:

Why? I still don't see what is wrong with the code or why it would be an improvement to use BigDecimals.

Citing ​http://www.cygnus-software.com/papers/comparingfloats/comparingfloats.htm :
"""
In other words, if you do a calculation and then do this comparison:

if (result == expectedResult)

then it is unlikely that the comparison will be true.
"""

Using better comparison (using floating point grade comparison) could led to better cache efficiency. Or we can go with something less complex than fp, like BigDecimals. I'll investigate that further with profiler though.

It would be a nice to have a modular structure, where you can plug in your own cache implementation. Then I see no technical reason to have JCS dependencies in jmapviewer.

On the other hand, why not. (As long as you can still compile jmapviewer without the JCS module.)

I tried to keep cache separate from all the other stuff, so only:

• ....imagery.TMSCachedTileJob
• ....imagery.TMSCachedTileLoader

Serve as a bridge between generic cache, and TMSLayer. Though - it bridges to TMSLayer and not to any of ...jmapviewer.* class.

[wiktorn]

I've attached patch that contains also changes in jmapviewer. As you will see, I've removed cached version of loader in jmapviewer.

Let me know, if it's OK, to include JCS in jmapviewer. I can make it so, that you can still compile jmapviewer without JCS, provided that developer will exclude JCS specific classes from his compile task.

[bastiK]

Replying to wiktorn:

I've attached patch that contains also changes in jmapviewer. As you will see, I've removed cached version of loader in jmapviewer.

Thanks for the patch! I'll need a little more time to review.

Let me know, if it's OK, to include JCS in jmapviewer.

Why do you think it would be better to include it in jmapviewer?

I can make it so, that you can still compile jmapviewer without JCS, provided that developer will exclude JCS specific classes from his compile task.

Sounds good.

[wiktorn]

Replying to bastiK:

Let me know, if it's OK, to include JCS in jmapviewer.

Why do you think it would be better to include it in jmapviewer?

Having JCS within jmapviewer tree, we will maintain functionality of having both - standard tile loader and cached tile loader. I could then reuse JobDispatcher, but I'd like then to change some of the interfaces (especially - remove extends Runnable from TileJob.

The drawback is growth of jmapviewer, with a bit of unrelated classes responsible for caching of any remote resource.

[bastiK]

Okay then. Btw., is there a reason we have a custom JobDispatcher class and not use java.util.concurrent.Executors?

[wiktorn]

Replying to bastiK:

Okay then. Btw., is there a reason we have a custom JobDispatcher class and not use java.util.concurrent.Executors?

Hm.. Not really . I'll udate to use java.util.concurrent.ThreadPoolExecutor with LIFO queue

[wiktorn]

Now I use ThreadPoolExecutor with quite small queue, so loader is concentrated on loading visible tiles.

How far I've checked, it's working properly both in slippy map chooser, as well as in TMSLayer, even when I select quite a few layers. Fixed few other bugs.

[bastiK]

Replying to wiktorn:

Now I use ThreadPoolExecutor with quite small queue, so loader is concentrated on loading visible tiles.

How far I've checked, it's working properly both in slippy map chooser, as well as in TMSLayer, even when I select quite a few layers. Fixed few other bugs.

LinkedBlockingDeque rejects new elements, rather than dropping the oldest. I doubt this the intended behavior. Also keep in mind that this is mostly depends on the screen resolution. For example on a large 1600x2560 screen, you can have up to 77 tiles at one time. What we could do instead, is drop requests that are off screen, after the user panned or zoomed. (Probably for another ticket.)

[wiktorn]

Replying to bastiK:

LinkedBlockingDeque rejects new elements, rather than dropping the oldest. I doubt this the intended behavior. Also keep in mind that this is mostly depends on the screen resolution. For example on a large 1600x2560 screen, you can have up to 77 tiles at one time. What we could do instead, is drop requests that are off screen, after the user panned or zoomed. (Probably for another ticket.)

Indeed. But there is no queue implementation within Collections Framework, that have this behavior, and I found that implementing it on my own, would gain little improvement. Current queue size is 5 elements (compared to 77 tiles on screen is really small) and with current redraw politics, that finishing download of a tile generates a redraw event in near future (100ms), I see that threads are constantly busy downloading tiles, although I have quite fast connection. So actually - the main queue are the tiles, that are struggling to get painted, instead of queue within Executor.

Long LIFO queue (say 100 or 1000 jobs), with enforcement of maximum size, that would remove objects on the bottom of the stack, would result in a behavior, that shown tiles are downloaded first what is good, but then, in background, we would be downloading tiles, that user is not seeing at all (as he changed the zoom or panned away from this tiles).

So I recommend to leave it for now as it is.

What we can change is whether we should stay with constant queue size of 5, or somewhere between 0.5 and 1.5 of THREAD_LIMIT

[bastiK]

Replying to wiktorn:

So actually - the main queue are the tiles, that are struggling to get painted, instead of queue within Executor.

I cannot imagine how painting in EDT can stop or delay download in background threads. This would be serious bug.

Long LIFO queue (say 100 or 1000 jobs), with enforcement of maximum size, that would remove objects on the bottom of the stack, would result in a behavior, that shown tiles are downloaded first what is good, but then, in background, we would be downloading tiles, that user is not seeing at all (as he changed the zoom or panned away from this tiles).

This isn't nice, but it works.

What we can change is whether we should stay with constant queue size of 5, or somewhere between 0.5 and 1.5 of THREAD_LIMIT

No, we cannot set a limit for the queue size: The user opens a TMS layer, 77 tiles are requested. There will be 10 active downloads, 5 in the queue and the remaining 62 are discarded. The user will end up with a screen mostly black.

[wiktorn]

Replying to bastiK:

What we can change is whether we should stay with constant queue size of 5, or somewhere between 0.5 and 1.5 of THREAD_LIMIT

No, we cannot set a limit for the queue size: The user opens a TMS layer, 77 tiles are requested. There will be 10 active downloads, 5 in the queue and the remaining 62 are discarded. The user will end up with a screen mostly black.

Apply the patch and see, that this is not the case.

When 77 tiles are requested, 15 jobs are created, rest is discarded. Then comes the first tile back (loaded), and a request in TMSLayer.tileLoadingFinished(...) to repaint the Map in near future (100ms).

As the map is redrawn in TMSLayer.paint(...), loadAllTiles is called, which puts new jobs into the queue, until queue if full.

Repeat until all tiles are fetched.

[bastiK]

Replying to wiktorn:

Replying to bastiK:

No, we cannot set a limit for the queue size: The user opens a TMS layer, 77 tiles are requested. There will be 10 active downloads, 5 in the queue and the remaining 62 are discarded. The user will end up with a screen mostly black.

Apply the patch and see, that this is not the case.

When 77 tiles are requested, 15 jobs are created, rest is discarded. Then comes the first tile back (loaded), and a request in TMSLayer.tileLoadingFinished(...) to repaint the Map in near future (100ms).

As the map is redrawn in TMSLayer.paint(...), loadAllTiles is called, which puts new jobs into the queue, until queue if full.

Repeat until all tiles are fetched.

Okay, this explains why it works, but I'm still not sold on this. If the user is panning and has full queue, then there will be 5 stale tiles requests in the queue. JOSM will download 5 tiles that are not on screen, before even starting to download the ones that are actually visible. On top of that, there is this 100ms thing which will delay download unnecessarily in case of a bounded queue. As an alternative solution, what about cleaning the queue before filling it with visible, but unloaded tiles? Anyway, I would like to get on with the JCS integration and would prefer if we could continue the queue improvement in a separate patch.

[bastiK]

Some comments:

• Logging to the command line is too much. Only forward errors and warnings by default, honor --debug or --trace flags if you want to output more. See the line FeatureAdapter.registerLoggingAdapter in Main.java for how logging can be translated to the normal JOSM output.
• AbstractDiskCacheAttributes.setDiskPath - should not emit error when path already exists.
• I like what you did in JCSCachedTileLoaderJob with the HEAD auto-detection. It would be slightly better to use the full URL template rather than the host for useHead. There could be a development server where various services share a host name.

[wiktorn]

Replying to bastiK:

Some comments:

• Logging to the command line is too much. Only forward errors and warnings by default, honor --debug or --trace flags if you want to output more. See the line FeatureAdapter.registerLoggingAdapter in Main.java for how logging can be translated to the normal JOSM output.

Corrected

• AbstractDiskCacheAttributes.setDiskPath - should not emit error when path already exists.

This is inside JCS framework. Is there any proper way, to patch external repositotires within JOSM build system? Or shall I try to fix this upstream?

• I like what you did in JCSCachedTileLoaderJob with the HEAD auto-detection. It would be slightly better to use the full URL template rather than the host for useHead. There could be a development server where various services share a host name.

Done.

[wiktorn]

PS. I have an idea, how to better solve logging issue. I'll update the patch in the evening.

[wiktorn]

revised v05

[wiktorn]

What is left to be done to get this patch integrated into the trunk? Adaption of imagery preferences and we're ready?

[bastiK]

The command line output is still too much. All JCS INFO level messages should only be shown be shown when --debug flag is present (i.e. this is FINE level for us).

There is sometimes a freeze of the GUI of up to 10-20 seconds (more often like 2 seconds). This means, when clicking the download button, the dialog window is completely gray for some time. Or if you open an imagery layer from the imagery menu, the menu will not close immediately, but stay open for some time (while GUI is unresponsive).

This is with a fresh preference, so I'm not sure what keeps JCS so busy. When repeating the action, the delay is usually gone. I don't mind the fact that there sometimes is a delay, but EDT should never be blocked for more than a split second.

[wiktorn]

Ok. Now I understand and corrected.

For GUI freezes - it might have been the case, that I had the cache on fast disk and didn't notice. I fixed that all accesses to JCS cache are in separate thread, as it may involve some I/O. I tested it on slow remote disk and haven't notice any hiccups.

This is separate to download thread pool, as this way, if we have some object in cache, we will return it, without waiting for a place in queue for downloads.

I've also updated preferences screen to accommodate changes.

[bastiK]

Replying to wiktorn:

Or shall I try to fix this upstream?

This would be best.

Replying to wiktorn:

For GUI freezes - it might have been the case, that I had the cache on fast disk and didn't notice. I fixed that all accesses to JCS cache are in separate thread, as it may involve some I/O. I tested it on slow remote disk and haven't notice any hiccups.

I still get the same symptoms. It may be related to initialization (note that this occurs with empty cache). Constructor of TMSCachedTileLoader is still called in EDT.

Another thing: Could you explain the relation of JCS memory cache and MemoryTileCache? Why do we need the JCS with 1000 keys, when the custom one is still used?

[wiktorn]

Replying to bastiK:

Replying to wiktorn:

Or shall I try to fix this upstream?

This would be best.

I've already created the ticket: ​https://issues.apache.org/jira/browse/JCS-145

Replying to wiktorn:

For GUI freezes - it might have been the case, that I had the cache on fast disk and didn't notice. I fixed that all accesses to JCS cache are in separate thread, as it may involve some I/O. I tested it on slow remote disk and haven't notice any hiccups.

I still get the same symptoms. It may be related to initialization (note that this occurs with empty cache). Constructor of TMSCachedTileLoader is still called in EDT.

I'll try to reproduce this by introducing artificial latency of 1 second between JOSM and remote disk. I'll check if I can reproduce this. If you could provide thread dump from this moment it would be great.

As a solution, maybe it would be the best, to move JCS initialization to "splash screen" so it will not introduce the problem later. Though having 20 second wait time at splash screen doesn't sound good either.

Another thing: Could you explain the relation of JCS memory cache and MemoryTileCache? Why do we need the JCS with 1000 keys, when the custom one is still used?

The custom one is used, because it caches BufferedImage's, as JCS caches only byte arrays. ImageIO.read sometimes takes quite a lot of time, so MemoryTileCache is still needed. We could drop memory cache in JCS at all, but it might be useful to other uses, and its easy to change its size.

I'll change the way preferences work, so TMS page will change only TMS settings and leave those in JCSCacheManager as property driven defaults for other uses.

[bastiK]

Replying to wiktorn:

Replying to bastiK:

Replying to wiktorn:

Or shall I try to fix this upstream?

This would be best.

I've already created the ticket: ​https://issues.apache.org/jira/browse/JCS-145

Great!

Replying to wiktorn:

For GUI freezes - it might have been the case, that I had the cache on fast disk and didn't notice. I fixed that all accesses to JCS cache are in separate thread, as it may involve some I/O. I tested it on slow remote disk and haven't notice any hiccups.

I still get the same symptoms. It may be related to initialization (note that this occurs with empty cache). Constructor of TMSCachedTileLoader is still called in EDT.

Now I kind of cannot reproduce anymore, i.e. it is usually less than 1 second. Will continue testing.

I'll try to reproduce this by introducing artificial latency of 1 second between JOSM and remote disk. I'll check if I can reproduce this. If you could provide thread dump from this moment it would be great.

Did the same test: Waited for external drive to go into hibernate mode, then opened the download window. It was different, in that the window did not show up until the drive was awake (few seconds). Previously I got an empty gray window for some time.

As a solution, maybe it would be the best, to move JCS initialization to "splash screen" so it will not introduce the problem later. Though having 20 second wait time at splash screen doesn't sound good either.

JCS initialization at startup may be a good idea, as it can be done concurrently along with the other startup tasks. The 20 second probably sounds more dramatic than it is: It happened only once and I suspect another process caused the system to stress out. The delay was shorter most of the time, but still quite noticeable.

Edit: Probably not needed, see below.

Another thing: Could you explain the relation of JCS memory cache and MemoryTileCache? Why do we need the JCS with 1000 keys, when the custom one is still used?

The custom one is used, because it caches BufferedImage's, as JCS caches only byte arrays. ImageIO.read sometimes takes quite a lot of time, so MemoryTileCache is still needed. We could drop memory cache in JCS at all, but it might be useful to other uses, and its easy to change its size.

We should worry about this when other uses arise, and not waste 30 MB of memory for nothing.

[bastiK]

Found something:

"AWT-EventQueue-0" prio=10 tid=0x00007f65c0521800 nid=0x25e7 runnable [0x00007f655fbbc000]
   java.lang.Thread.State: RUNNABLE
	at java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
	at java.net.InetAddress$1.lookupAllHostAddr(InetAddress.java:901)
	at java.net.InetAddress.getAddressesFromNameService(InetAddress.java:1293)
	at java.net.InetAddress.getAllByName0(InetAddress.java:1246)
	at java.net.InetAddress.getAllByName(InetAddress.java:1162)
	at java.net.InetAddress.getAllByName(InetAddress.java:1098)
	at java.net.InetAddress.getByName(InetAddress.java:1048)
	at java.net.URLStreamHandler.getHostAddress(URLStreamHandler.java:437)
	- locked <0x0000000785078670> (a sun.net.www.protocol.https.Handler)
	at java.net.URLStreamHandler.hashCode(URLStreamHandler.java:354)
	at java.net.URL.hashCode(URL.java:877)
	- locked <0x00000007d9a38dd0> (a java.net.URL)
	at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:333)
	at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:988)
	at org.openstreetmap.josm.data.cache.JCSCachedTileLoaderJob.submit(JCSCachedTileLoaderJob.java:143)
	- locked <0x00000007d9a31200> (a java.util.concurrent.ConcurrentHashMap)
	at org.openstreetmap.josm.data.imagery.TMSCachedTileLoaderJob.submit(TMSCachedTileLoaderJob.java:95)
	at org.openstreetmap.gui.jmapviewer.TileController.getTile(TileController.java:48)
	at org.openstreetmap.gui.jmapviewer.JMapViewer.paintComponent(JMapViewer.java:584)
	at javax.swing.JComponent.paintToOffscreen(JComponent.java:5216)
	at javax.swing.RepaintManager$PaintManager.paintDoubleBuffered(RepaintManager.java:1529)
	at javax.swing.RepaintManager$PaintManager.paint(RepaintManager.java:1452)
	at javax.swing.RepaintManager.paint(RepaintManager.java:1249)
	at javax.swing.JComponent.paint(JComponent.java:1032)
	at org.openstreetmap.josm.gui.bbox.SlippyMapBBoxChooser.paint(SlippyMapBBoxChooser.java:209)
	at org.openstreetmap.josm.gui.download.DownloadDialog.paint(DownloadDialog.java:166)
	at javax.swing.RepaintManager$3.run(RepaintManager.java:819)
	at javax.swing.RepaintManager$3.run(RepaintManager.java:796)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:796)
	at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:769)
	at javax.swing.RepaintManager.prePaintDirtyRegions(RepaintManager.java:718)
	at javax.swing.RepaintManager.access$1100(RepaintManager.java:62)
	at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1677)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:312)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:738)
	at java.awt.EventQueue.access$300(EventQueue.java:103)
	at java.awt.EventQueue$3.run(EventQueue.java:699)
	at java.awt.EventQueue$3.run(EventQueue.java:697)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:708)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:154)
	at java.awt.WaitDispatchSupport$2.run(WaitDispatchSupport.java:182)
	at java.awt.WaitDispatchSupport$4.run(WaitDispatchSupport.java:221)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.awt.WaitDispatchSupport.enter(WaitDispatchSupport.java:219)
	at java.awt.Dialog.show(Dialog.java:1082)
	at java.awt.Component.show(Component.java:1655)
	at java.awt.Component.setVisible(Component.java:1607)
	at java.awt.Window.setVisible(Window.java:1014)
	at java.awt.Dialog.setVisible(Dialog.java:1005)
	at org.openstreetmap.josm.gui.download.DownloadDialog.setVisible(DownloadDialog.java:424)
	at org.openstreetmap.josm.actions.DownloadAction.actionPerformed(DownloadAction.java:43)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2018)
	at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2341)
	at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
	at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
	at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:252)
	at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
	at java.awt.Component.processMouseEvent(Component.java:6516)
	at javax.swing.JComponent.processMouseEvent(JComponent.java:3312)
	at java.awt.Component.processEvent(Component.java:6281)
	at java.awt.Container.processEvent(Container.java:2229)
	at java.awt.Component.dispatchEventImpl(Component.java:4872)
	at java.awt.Container.dispatchEventImpl(Container.java:2287)
	at java.awt.Component.dispatchEvent(Component.java:4698)
	at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4832)
	at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4492)
	at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4422)
	at java.awt.Container.dispatchEventImpl(Container.java:2273)
	at java.awt.Window.dispatchEventImpl(Window.java:2719)
	at java.awt.Component.dispatchEvent(Component.java:4698)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:740)
	at java.awt.EventQueue.access$300(EventQueue.java:103)
	at java.awt.EventQueue$3.run(EventQueue.java:699)
	at java.awt.EventQueue$3.run(EventQueue.java:697)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
	at java.awt.EventQueue$4.run(EventQueue.java:713)
	at java.awt.EventQueue$4.run(EventQueue.java:711)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:710)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)

And: ​http://stackoverflow.com/questions/2348399/why-does-java-net-urls-hashcode-resolve-the-host-to-an-ip

So I guess it's not my slow hard disk, but my shaky network connection. :)

[wiktorn]

Replying to bastiK:

Now I kind of cannot reproduce anymore, i.e. it is usually less than 1 second. Will continue testing.

Thank you for your stack trace. It was very helpful. Instead of using java.net.URL as Map keys, I use now Strings, so the problem is solved. So I didn't move any JCS initalizations to splash screen.

We should worry about this when other uses arise, and not waste 30 MB of memory for nothing.

I changed a bit the preferences. Now, if any other JCS client will come, it will give some memory cache to other regions, but for TMS - the region has 0 memory elements size limit.

[bastiK]

The delay problem is fixed now, thanks!

• JCSCacheManager does not respect the path set with JCSCacheManager.setCacheParams. I end up with both cache/jcs and cache/tms, but only the first has content.
• You have defined DOWNLOAD_JOB_DISPATCHER both in TMSCachedTileLoaderJob and JCSCachedTileLoaderJob, is that intended? One could use the convenience function Collections.asLifoQueue to make a LIFO.
• Should we revert the changes related to JCSCachedTileLoaderJob.CACHE_ACCESS_EXECUTOR for sake of code clarity? I don't think it is actually needed.

Minor stuff:

• ICachedLoaderJob: Did you plan to use generics for the value? Doc should be cleaned up.
• CacheEntryAttributes has unused fields

[wiktorn]

Replying to bastiK:

• JCSCacheManager does not respect the path set with JCSCacheManager.setCacheParams. I end up with both cache/jcs and cache/tms, but only the first has content.

This should be fixed now. Though to change cache directory, one needs to call getCache(...) with different parameters.

• You have defined DOWNLOAD_JOB_DISPATCHER both in TMSCachedTileLoaderJob and JCSCachedTileLoaderJob, is that intended? One could use the convenience function Collections.asLifoQueue to make a LIFO.

DOWNLOAD_JOB_DISPATCHER is defined twice indeed. I made the comment in TMSCachedTileLoaderJob more clear, why it's done so.

Collections.asLifoQueue returns Collection, whereas Executors need BlockingQueue

• Should we revert the changes related to JCSCachedTileLoaderJob.CACHE_ACCESS_EXECUTOR for sake of code clarity? I don't think it is actually needed.

Reverted. I didn't did that in first place, because of the I/O happening in the EDT thread. But indeed this introduced too much complication into the code.

Minor stuff:

• ICachedLoaderJob: Did you plan to use generics for the value? Doc should be cleaned up.
• CacheEntryAttributes has unused fields

Corrected.

[bastiK]

• When JOSM is closed, I always end up with an empty cache/tms/TMS.data file (0 Byte).
• Could you test stability against crashes, e.g. out of memory exception? Will the cache get corrupted / reset?
• Are you sure we can remove the classes OsmFileCacheTileLoader and TMSFileCacheTileLoader? This means other users of jmapviewer will no longer have a caching mechanism.

We are currently in stabilization, the patch cannot be integrated before next release.

[wiktorn]

Replying to bastiK:

• When JOSM is closed, I always end up with an empty cache/tms/TMS.data file (0 Byte).

This is due the bug in JCS, that I've just nailed down (4 hours to find one missing line. Hate that).
​https://issues.apache.org/jira/browse/JCS-146

From my experience, it wasn't always the case, that the file was corrupted at the end of JOSM session, but still too often.

I'm not satisfied with reaction times on JCS side, shall I extend Ant build file with ant patch task for JCS? This will require patch binary in path though :-(

• Could you test stability against crashes, e.g. out of memory exception? Will the cache get corrupted / reset?

If any corruption is seen within cache file, the cache is reset. So during OOM, if some write will fail, then the cache will reset itself.

• Are you sure we can remove the classes OsmFileCacheTileLoader and TMSFileCacheTileLoader? This means other users of jmapviewer will no longer have a caching mechanism.

Indeed. They will not. But here is not an easy choice:

• keep cache code duplicated between jmapviewer and JOSM, so jmapviewer will not depend on JCS
• integrate the code, but jmapviewer will depend on JCS, and will come with overload of HTTP request cached in JCS
• do the reflection tricks, so there will be no compile dependency on JCS, but on-demand loading, keep the JCS and "adaptor classes" in JOSM tree, but in the end - we will still have jmapviewer, that has no caching mechanism

As my feeling is, that jmapviewer is simple solution, that could be plugged into any application, I think that wisest is just to resign from caching in jmapviewer.

We are currently in stabilization, the patch cannot be integrated before next release.

Ok, then lets aim for next release.

[bastiK]

Replying to wiktorn:

Replying to bastiK:

• When JOSM is closed, I always end up with an empty cache/tms/TMS.data file (0 Byte).

This is due the bug in JCS, that I've just nailed down (4 hours to find one missing line. Hate that).
​https://issues.apache.org/jira/browse/JCS-146

Wow, this looks pretty difficult to find.

I'm not satisfied with reaction times on JCS side, shall I extend Ant build file with ant patch task for JCS?

No, we will simply copy the JCS source code into our repository and patch it there.

• Are you sure we can remove the classes OsmFileCacheTileLoader and TMSFileCacheTileLoader? This means other users of jmapviewer will no longer have a caching mechanism.

As my feeling is, that jmapviewer is simple solution, that could be plugged into any application, I think that wisest is just to resign from caching in jmapviewer.

Okay we can keep it as is and see if anyone complains... (To fix this, my rough idea would be to have an abstract cache interface, which is implemented both by JCS and the simple disk cache. The code would only depend on this interface and could live in the jmapviewer repository. For JOSM we "plug in" JCS instead of the disk cache.)

Another sketchy idea:

To get rid of LRUMemoryCache, we could customize the serialization in CacheEntry: I played a bit with a very basic cache config and it seems that a cache element gets serialized at most once and this happens directly after calling put. And it gets deserialized at most once, namely when it is loaded from disk cache into memory cache. If this is true, we could do something like this:

public class CacheEntry implements Serializable {
    private static final long serialVersionUID = 1L; //version
    private byte[] content;
    private BufferedImage img;

    /**
     * @param content of the cache entry
     */
    public CacheEntry(byte[] content) {
        this.content = content;
    }

    public BufferedImage getImage() throws IOException {
        if (img == null) {
            if (content == null) throw new AssertionError();
            img = ImageIO.read(new ByteArrayInputStream(content));
        }
        return img;
    }

   private void writeObject(java.io.ObjectOutputStream stream)
            throws IOException {
        if (content == null) throw new AssertionError();
        stream.writeObject(content);
        getImage();
        content = null;
    }

    private void readObject(java.io.ObjectInputStream stream)
            throws IOException, ClassNotFoundException {
        content = (byte[]) stream.readObject();
        getImage();
        content = null;
    }

}

[wiktorn]

Brilliant idea. Instead of customizing serialization in CacheEntry, I introduced BufferedImageCacheEntry, that gives desired behaviour. This is now used by TMSLayer, but for other uses (Mapillary images), plain CacheEntry might be enough.

I tried to verify the performance impact of the change, and it doesn't do much difference. ImageIO.read is moved to the other place (still in EDT thread) but I do not see an easy escape from this.

I havent removed MemoryTileCache class, as I tried to be a bit conservative with this patch, and do allow some other tileLoader implementation, that might actually not have such good memory cache. So as a fallback, there is still MemoryTileCache available.

I could do some more cleaning in TMSLayer, to get rid of MemoryTileCache, and rework, how it cooperates with tileLoader, but this might be less backward compatible and will require, that all future tile loaders would give memory cache at least. I'm not so sure, if this is a way to go.

[bastiK]

Replying to wiktorn:

Instead of customizing serialization in CacheEntry, I introduced BufferedImageCacheEntry, that gives desired behaviour. This is now used by TMSLayer, but for other uses (Mapillary images), plain CacheEntry might be enough.

Please make sure, the byte data can be garbage collected after it is serialized. Otherwise we are wasting memory by keeping the image data twice.

It seems, MemoryTileCache is still used by SlippyMapBBoxChooser (download dialog). I would be nicer, if this class wasn't instantiated at all, and we had a JCS-only cache.

I tried to verify the performance impact of the change, and it doesn't do much difference. ImageIO.read is moved to the other place (still in EDT thread) but I do not see an easy escape from this.

I havent removed MemoryTileCache class, as I tried to be a bit conservative with this patch, and do allow some other tileLoader implementation, that might actually not have such good memory cache. So as a fallback, there is still MemoryTileCache available.

I don't understand this concern. Overall, I have the feeling that the structure is a little more complicated than it needs to be. This isn't your fault, it was like this before. I would welcome any change, that simplifies the code and makes it easier to understand. It is good to think ahead, but you don't have to add parameters and hooks for every conceivable future use case. We have a very short development circle and can add any abstractions and extensions as soon as they are needed.

I could do some more cleaning in TMSLayer, to get rid of MemoryTileCache, and rework, how it cooperates with tileLoader, but this might be less backward compatible and will require, that all future tile loaders would give memory cache at least. I'm not so sure, if this is a way to go.

Sounds good, but if it could delay the integration of the current patch, then I would prefer to finish this first.

[wiktorn]

Replying to bastiK:

Replying to wiktorn:

Please make sure, the byte data can be garbage collected after it is serialized. Otherwise we are wasting memory by keeping the image data twice.

What I was thinking about was something like that:

    public BufferedImage getImage() throws IOException {
        if (img != null)
            return img;
        synchronized(this) {
            if (img != null)
                return img;
            img = ImageIO.read(new ByteArrayInputStream(getContent()));
            content = null;
        }
        return img;
    }
    
    private void writeObject(java.io.ObjectOutputStream out) throws IOException {
        synchronized (this) {
            if (content == null && img != null) {
                ByteArrayOutputStream restoredData = new ByteArrayOutputStream();
                ImageIO.write(img, "png", restoredData);
                content = restoredData.toByteArray();
            }
            out.writeObject(this);
        }
    }

But I see one issue with this:

• This way, I convert all cache entries to PNG format, even if they were provided as JPEG, so it may introduce additional loss of quality of image, what I'd like to avoid

I could just set the content = null and pray, that the memory object once it is saved to disk, will never get updated with memory representation if I'll change. This is actually the case now, as we use DiskUsagePattern.UPDATE, so I have a felling, that it might be a fragile solution.

We can go with this simple hack, and I can just comment it verbose'ly, so if any update in JCS behaviour will happen in future, I hope that solving the bug will be much easier.

Do you know any better way to convert from Image back to byte stream preserving the original format?

By the way - I contacted maintainers of JCS and they promised, that they'll take a look into my patches soon.

[wiktorn]

And one thing more - MemoryTileCache is also used in JMapViewer. So my guess is, that this class should stay. I'll just remove any references to it from org.openstreetmap.josm.* packages.

[bastiK]

Replying to wiktorn:

What I was thinking about was something like that:

...

But I see one issue with this:

• This way, I convert all cache entries to PNG format, even if they were provided as JPEG, so it may introduce additional loss of quality of image, what I'd like to avoid

I think we should keep the original image data bit-identical as downloaded from the server, so this is not an option.

I could just set the content = null and pray, that the memory object once it is saved to disk, will never get updated with memory representation if I'll change. This is actually the case now, as we use DiskUsagePattern.UPDATE, so I have a felling, that it might be a fragile solution.

We can go with this simple hack, and I can just comment it verbose'ly, so if any update in JCS behaviour will happen in future, I hope that solving the bug will be much easier.

I see nothing wrong with this. We can throw an AssertionError as soon as this assumption is violated, so we will know at once. If JCS changes, we can revert to the old version and report it as a regression. After all, it is wasteful and unnecessary to do the serialization twice.

Do you know any better way to convert from Image back to byte stream preserving the original format?

I doubt it is possible.

By the way - I contacted maintainers of JCS and they promised, that they'll take a look into my patches soon.

Great!

Replying to wiktorn:

And one thing more - MemoryTileCache is also used in JMapViewer. So my guess is, that this class should stay. I'll just remove any references to it from org.openstreetmap.josm.* packages.

Agreed.

[wiktorn]

Replying to bastiK:

I see nothing wrong with this. We can throw an AssertionError as soon as this assumption is violated, so we will know at once. If JCS changes, we can revert to the old version and report it as a regression. After all, it is wasteful and unnecessary to do the serialization twice.

I've made the assertion and also made verbose comment about it.

For removing the MemoryTileCache from TMSLayer and SlippyMapBBoxChooser, I'd like first to get this patch in, and then work further. This is already huge patch and keeping it in shape, is an extra effort for me.

[wiktorn]

Issues in JCS resolved:
​https://issues.apache.org/jira/browse/JCS-144 - BlockDiskCache deadlocks/livelocks
​https://issues.apache.org/jira/browse/JCS-145 - logging error when cache path already exists
​https://issues.apache.org/jira/browse/JCS-146 - IndexedDiskCache corruption

[bastiK]

Replying to wiktorn:

Issues in JCS resolved:
​https://issues.apache.org/jira/browse/JCS-144 - BlockDiskCache deadlocks/livelocks
​https://issues.apache.org/jira/browse/JCS-145 - logging error when cache path already exists
​https://issues.apache.org/jira/browse/JCS-146 - IndexedDiskCache corruption

This is great!

Sorry for the delay, I'll look into it as soon as possible.

[wiktorn]

Corrected NullPointerException on "no tiles at this zoom level"

[bastiK]

In 8168/josm:

see #11216 - Limit size of TMS cache (patch by wiktorn)

[bastiK]

In [o31077]:

see ​#11216 - Limit size of TMS cache (patch by wiktorn)

[Don-vip]

​http://donvip.fr/jenkins/job/JOSM/1987/ Jenkins build is broken

[wiktorn]

Indeed. JCS just commited some changes. Fix attached.

[bastiK]

In 8172/josm:

see #11216 - fix IndexedDiskCacheManager (patch by wiktorn)

[wiktorn]

Patch for NPE when BingAttribution is not loaded, and for not showing tiles at overzoom

[Don-vip]

In 8173/josm:

see #11216 - update Eclipse classpath and build.xml

[bastiK]

In 8174/josm:

see #11216 - fixes NPE when BingAttribution is not loaded, and for not showing tiles at overzoom (patch by wiktorn)

[bastiK]

related tickets:
#11300 - block by start download - first step after update
#11317 - tiles flicker
#11319 - lock JCS cache directory
#11327 - Add possibility to clear from cache only specified TileSource

@wiktorn: Thanks for the quick response to the reported problems. A few bugs are expected after such a major change, so don't worry. :)

[Larry0ua]

Hi guys! Is the tile cache info on latest josm related to this ticket? If so, then I probably hit some bug related to the cache management. Now when some area is downloaded, josm starts mass downloading of bing tiles and hangs till download ends or out of memory happens. Please check. I am not sure how to make a ticket from this behavior, but if more investigation is needed, please tell.

[stoecker]

#11320 is also related

[stoecker]

@wiktorn:
Congratulations. Your change created the largest disturbances for some time. Got really boring before :-)

But as bastiK said, that was to be expected. If we can get these issues solved till end of month it would be fine, so we can keep the schedule. Otherwise we also can skip one release.

@team:
We should add a cleanup function for old caches. They can be really large sometimes. Also some other tickets have probably been closed by that change (I remember one about cache unification).

[Don-vip]

ElevationProfile plugin needs update: ​http://donvip.fr/jenkins/job/JOSM-Plugins/1072/console

[wiktorn]

Elevation profile fix

[bastiK]

Replying to Larry0ua:

Hi guys! Is the tile cache info on latest josm related to this ticket? If so, then I probably hit some bug related to the cache management. Now when some area is downloaded, josm starts mass downloading of bing tiles and hangs till download ends or out of memory happens. Please check. I am not sure how to make a ticket from this behavior, but if more investigation is needed, please tell.

This sounds like a serious problem. Could you please create a separate ticket? What do you mean by "JOSM hangs"? The download should be performed in the background and not cause JOSM to hang. It would at least be interesting to know, if it also happens for imagery sources other than Bing and which tiles it downloads. They must be out of bounds or at wrong zoom level, otherwise it would be expected behavior. Maybe you get some insight by starting it from the command line wiht --debug option.

[wiktorn]

Replying to bastiK:

@wiktorn: Thanks for the quick response to the reported problems. A few bugs are expected after such a major change, so don't worry. :)

Replying to stoecker:

@wiktorn:
Congratulations. Your change created the largest disturbances for some time. Got really boring before :-)

But as bastiK said, that was to be expected. If we can get these issues solved till end of month it would be fine, so we can keep the schedule. Otherwise we also can skip one release.

Still, I've created more disturbance, than I intended too. I hope that what is left, is more less in good standing for next release. If not however, I'll be not available to work on JOSM exactly till date of upcoming release.

I already have implemented some improvements to my work-pipe, which I hope that will improve the quality of the next patches, and I have some thoughts, what I could do better this time.

Anyway - for May I've already have stashed moving WMS layer to JCS, I also would like to switch to proper disk size limits thanks to recent changes to JCS.

[stoecker]

I rather have some new stuff and trouble than no trouble and no progress. And everybody of us did more serious issues already. Maybe we should make a "who's change caused most tickets" hitlist :-)

[Larry0ua]

Replying to bastiK:

Replying to Larry0ua:

Hi guys! Is the tile cache info on latest josm related to this ticket? If so, then I probably hit some bug related to the cache management. Now when some area is downloaded, josm starts mass downloading of bing tiles and hangs till download ends or out of memory happens. Please check. I am not sure how to make a ticket from this behavior, but if more investigation is needed, please tell.

This sounds like a serious problem. Could you please create a separate ticket? What do you mean by "JOSM hangs"? The download should be performed in the background and not cause JOSM to hang. It would at least be interesting to know, if it also happens for imagery sources other than Bing and which tiles it downloads. They must be out of bounds or at wrong zoom level, otherwise it would be expected behavior. Maybe you get some insight by starting it from the command line wiht --debug option.

I am sure that time freezes were connected to the GC, not to the application itself. Anyway, now I cannot reproduce that problem on latest jar. Will keep you posted.

[bastiK]

Some open points remaining, most importantly WMS migration.

[bastiK]

In 8277/josm:

see #11216 - fixed netbeans project

[Don-vip]

skip milestone 15.06

[wiktorn]

In 8526/josm added JCS support for WMS.

[Don-vip]

Milestone renamed

[brycenesbitt]

I'm here looking for a way to limit the size of the TMS cache. This is a reminder to enable to preference item mention above, it it's not already done.

[wiktorn]

It's long time there. See the marking on the screenshot: