Concurrency MemoryDataStore

Andrea Aime added a comment - 25/Jun/11 11:35 AM We cannot use a concurrent hash map for memory data store. Memory data store is used mostly in tests and we have to preserve iteration order. The patch should be rewritten using synchronization statements, or some flags in the store that decide what kind of backing store to use, and then alternatively create a synchronized linked hash map, or a concurrent hash map (with the first option as the default, to avoid breaking existing code)

Frank Gasdorf added a comment - 25/Jun/11 1:06 PM Wondering that DataStores should preserve iteration order. IMHO it's not guaranteed (e.g. database selection order). Why should it be required for MemoryDataStore? I guess you're talking about test cases using MemoryDataStores, that could be implemented in a way the resulting order of features is relevant. I would prefer to rewrite test cases if any test breaks .. but I've no clue at the moment where MemoryDataStore is used. But I remember that there where no tests that were broken after changing to ConcurrentHashMap in Mai 2009.

Andrea Aime added a comment - 25/Jun/11 2:22 PM You did not see them because you probably were still running with java 5, but use java 6 and you'll see a different iteration order. As far as I know there is quite a bit of them around the code base, if you want to hunt them down and fix them feel free to :)

Frank Gasdorf added a comment - 25/Jun/11 3:59 PM Again, I've found no indicate that a DataStore or even a FeatureReader returns the Features always in the same order. And I guess it's not guaranteed that the order within ordered Features is always the same if the values of features for the order criteria is equal. For me it would be a bug in test case if it's assumed to get features in the order they where added.

Andrea Aime added a comment - 25/Jun/11 4:44 PM Frank, I think we are talking past each other. You are correctly saying that a test assuming a certain order is broken, I agree. I am saying that unless you go and fix them there is little likeliness that someone else would. Correctness is one thing, resourcing is another, if you just go and break the build the first reaction will be to revert your change unless a corresponding fix for the build follows very quickly.

Andrea Aime added a comment - 26/Jun/11 4:06 AM Btw, another observation. If people are using the store to build map with overlapping features having a predictable iteration order is important to get the right output. Again, a solution were a flag controls what kind of map to use to privilege scalability or iteration order guarantee is likely the best option

Jody Garnett added a comment - 26/Jun/11 4:09 AM Hi Frank; the WFS filter specification has SortBy and the concept of natural ordering. You are correct that order dependence is not a general requirement of DataStore / FeatureReader --> It is just something that MemoryDataStore does to make testing easier. If you would like there are a number of different datastore implementations you can use that perform a lot better than MemoryDataStore. We added them specifically because people kept assuming that MemoryDataStore would be a "fast" way to do things - and nothing could be further than the truth. For details look up performance here in the docs: - http://docs.geotools.org/latest/userguide/library/main/collection.html#performance-options In particular one of them is a clean wrapper around a collection of your choice. You could probably do an ever more efficient one (with less locking) if you knew that ConcurrentHashMap was being used. In most cases the read only one using a spatial index is the way to go.

Jody Garnett added a comment - 26/Jun/11 4:13 AM Frank we should probably get discussion back to your actual bug report. Can I ask if you were using a transaction for the modification? Or using transaction AUTO_COMMIT? I don't actually know anything about org.geotools.gui.swing.map.map2d.stream.TempMemoryDataStore

Andrea Aime added a comment - 26/Jun/11 4:14 AM Agreed, would be nice to have a "fast memory data store" instead of the "example memory data store" that uses a spatial index and packed coordinate sequences to reduce the memory usage. If the store is mostly read only there could be other ways to reduce memory consumption, like making shallow clones of the internal features during reads (just thin wrappers pointing to the original feature).

Frank Gasdorf added a comment - 27/Jun/11 3:57 AM - edited @Andrea : I absolutely agree and your arguments are logical. I don't want to break tests in the library itself and in projects based on geotools as well. And like you mentioned we do not know about anything about the tests, therefore big plus for pointing this out! I do not have much resources to check the whole library too. @Jody : We used AUTO_COMMIT Option in this project 2 years ago, based on 2.5.x, I remember that we used 2.5.8 finally. The TempMemoryDataStore can be here : found http://svn.osgeo.org/geotools/branches/2.5.x/modules/unsupported/widgets-swing-pending/src/main/java/org/geotools/gui/swing/map/map2d/stream/TempMemoryDataStore.java I do agree with the spatial index, would be a good enhancement. For me it's not important to get it fixed on 2.5.x branch. I'd like to get it improved generally. I'll have a look at the collection docs Jody mentioned. Just an idea: what about using ContentDataStore and only fix this on trunk?

Andrea Aime added a comment - 27/Jun/11 4:52 AM Having a memory data store overhaul with spatial index, good concurrency, and based on content data store would be nice indeed.

Jody Garnett added a comment - 27/Jun/11 8:12 PM I think it is already done. There is a indexed based feature collection; and a special FeatureSource that wraps around it... Frank if you needed to back port you could use AbstractDataStore in order to handle the DataStore/FeatureSource wrapper.