Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added parallel array tests for parallel access #23

Merged
merged 5 commits into from
Apr 6, 2020

Conversation

juliusfriedman
Copy link

@juliusfriedman juliusfriedman commented Mar 9, 2020

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

@juliusfriedman
Copy link
Author

juliusfriedman commented Mar 9, 2020

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
Copy link
Author

juliusfriedman commented Mar 10, 2020

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
Copy link
Author

juliusfriedman commented Mar 12, 2020

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
Copy link
Author

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

@xivk
Copy link
Contributor

xivk commented Apr 6, 2020

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.

@xivk xivk merged commit f087b93 into itinero:develop Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants