-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
Implemented preview observables for lists and caches. #199
Conversation
Use reader-writer locks. See issue #194
I've finally have time to consider these changes. It may take some time to digest but I will get back to you as soon as I can |
This is a well thought out PR and I can see you have been very thorough and written good unit tests. Before I review the code in detail there are some immediate thoughts which spring to mind. Also don't worry about the size of the PR at least for now as the number of files touched is due to the nature of what you are trying to achieve. Performance For a long time I have been meaning to add benchmarks. #121 has been open since July! Perhaps it would be a good idea for benchmarks to be added for source cache and list before this PR goes in. That way we can get a better measure of of the impact of changing the locking mechanism. Maybe doing so should be my priority. Usage of Preview I am curious about how preview should be used and do we need to restrict it to previewing direct changes only. For example the API allows the chaining of operators var myPreview = myCache.Preview().Filter(aFilter).Transform(someSelector) Should this be allowed? If so we need to consider the implications. Could you also please give me a concrete example of how you use the concept with ReactiveList. Design Depending on your answers to the above, we may need to think about introducing a distinct API for the preview operator ie. perhaps a specialised PreviewChangeSet which could have limited operators if existing operators are not permitted. |
Here is a more concrete example of where Imagine you have a graph-like datastructure describing a company. A company has departments, which in turn have people. Now, let's say that arbitrary people in the company (cross-department) can have relationships defined between them. People, departments and relationships can be (re)moved and added at any point. We should be able to query both all relationships and the relationships of a specific person efficiently. This could be implemented using the following set of classes:
Now when a person is removed from a department, his/her relationships should be removed both from the person and the company object. Here is what happens now with DynamicData:
Solution:
|
I don't see why not. The original changeset object should not be modified by the operators, the operators should not attempt to modify the cache and the operators should not rely on the current state of the cache (only on the changeset). As long as these conditions are satisfied, everything should work as it would with Connect(). |
I have been thinking about this for some time now and what you say make clear sense. This is a very powerful feature and will greatly benefit many consumers of this library. My only concern with the implementation is what the performance impact of the changed locking mechanism would be. I suspect for most cases the difference would be negligible as my experience tells me there is little lock contention at the cache level in most cases. However the impact could be noticeable for grouping or transformtotree owing to the number of child list / cache sources initially created. As I suggested before we will need to add some performance bench marks first. Currently I am working on a large change but would happily drop it to get some basic benchmarks in asap. Additionally, you also suggested breaking the PR up into smaller chunks. Are you happy to do that? Perhaps change the locking for SourceList only (without the preview). Doing so would enable us to easily benchmark the locking. Then assuming all is good, the rest can quickly follow |
I opened PR #211. |
This is a pull request for issue #194.
Sorry for the wall of text :) I also now realize that this has become a fairly large pull request. I can split it up into multiple smaller ones if you would prefer so.
Preview()
toIConnectableCache
andIObservableList
, which returns anIObservable<IChangeSet<T>>
. This observable allows its observers to view changes before they are applied to the list/cache.ICollectionSubject
. These provide methods to runOnComplete
andOnError
on these collections. I found it unclear whether or not the preview event should be included in the implementation of these methods. However, after inspecting their usage, I found that there is only one line where it is used: ObservableChangeSet:398. Similar methods do not invoke this method and I don't really see why its there. All tests run fine after removing it. Furthermore, I also don't see any good reason for external users of the collections to call OnComplete/OnError as this is part of the internal state of the collection. Completing the observables can be done by disposing the collection object anyway.A note about locks:
ReaderWriter
s withReaderWriterLockSlim
. It turns out this was not really necessary for the implementation of thePreview()
event. However, a large advantage of this is that multiple threads can read the collection simultaneously as long as no update is taking place. This was not possible with the old lock system and should improve concurrent performance. - After implementing usage ofReaderWriterLockSlim
I found that some of the tests forTreeBuilder
were failing. This is becauseTreeBuilder
first disposes nodes that are updated/removed, and then reads/writes from/to the lists of children in these nodes. Since the dispose method also dispose theReaderWriterLockSlim
for these lists, the lists were inaccessible after disposal and so exceptions would occur. At first I thought they were only being read, so I implemented and usedTwoStageRWLock
instead ofReaderWriterLockSlim
. This allows reading, but not writing, after disposal. However, it turns out that these nodes are also being written to after disposal. I then solved that by simply disposing the nodes after the setparent operation has run, instead of before.