search

itinero / reminiscence

A library for cross-platform memory mapping.
MIT License
9 stars 6 forks source link

Added parallel array tests for parallel access #23

Closed juliusfriedman closed 4 years ago

juliusfriedman commented 4 years ago

ArrayTests.TestParallelAccess() is passing so the issue seems to be with Itinero and not in this library.

juliusfriedman commented 4 years ago

Perhaps there should also be some parallel tests for Index, https://github.com/itinero/reminiscence/blob/develop/test/Reminiscence.Tests/Indexes/IndexTests.cs

I could add some parallel tests here just to ensure I don't find anything if your interested.

juliusfriedman commented 4 years ago

I updated the tests to show a failure in Index.Get when used in parallel, it's strange because when enumerated first and then in parallel it does not happen. I will look into this if I can. I am not sure if your intention is to allow multiple readers but based on the test passing when I enumerate the first time I would say this is a bug.

It seems that when ReadFrom is called the position is seeked in some cases, but it is never seeked back to where it was originally, this either results in reading past the end of the stream or reading an index which is not correct.

There are a few places IMHO which would need to be fixed and they are all from MappedAccessor<T> @ public abstract long ReadFrom(Stream stream, long position, ref T structure);

Mostly the test is getting the bad results from MemoryMapDelegate.ReadFromString, locking the stream or using Monitor.Enter doesn't seem to help in the delegate however locking the index in the test prevents the exception.

juliusfriedman commented 4 years ago

I was able to resolve the Parallel issue by using a lock in the Get method on Index, I still find it strange that if enumerated before hand that no lock is required for subsequent parallel access as I find no caching code.

Let me know if you don't think this is a good idea.

Based on my tests I can remove the lock in ProfileAndSpeedCache in Itinero now since this code is locking

juliusfriedman commented 4 years ago

@xivk, please let me know when you get a chance to look at this as well as the other PR's submitted

xivk commented 4 years ago

hey @juliusfriedman the lock fixes the problem but I find it more up to the user to behave properly in using the index. The lock adds a performance penalty I think. It could affect Itinero negatively because it gets attributes from an index when building a route and this implies one or more locks over different objects to get one attribute key or value.

I would prefer in the long run to make this optional by default or to allow users to set their own synchronization object making it usable in Itinero without too much modifications and/or preformance penalties I believe. I will accept this pull request, we'll handle any performance impact (if any) when it becomes an issue and make this optional if needed.