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

Implemented preview observables for lists and caches. #199

Closed
wants to merge 1 commit into from
Closed

Implemented preview observables for lists and caches. #199

wants to merge 1 commit into from

Conversation

Wouterdek
Copy link
Contributor

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.

  • Adds Preview() to IConnectableCache and IObservableList, which returns an IObservable<IChangeSet<T>>. This observable allows its observers to view changes before they are applied to the list/cache.
  • The preview is implemented as follows: If the preview observable has no subscribers, the editing process works as usual. If it has one or more subscribers, then the list/cache is first copied. Then, the change is applied to the main list/cache. Next, the set of changes is obtained from the modified list/cache. The modified datastructure is then swapped with the copy so that the old state is visible again. The preview event is then run. Finally, the modified datastructure is swapped with the old copy once more so that the new state becomes visible and the change event is triggered.
  • During implementation, I found that both the list and the cache implement ICollectionSubject. These provide methods to run OnComplete and OnError 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.
  • Some update action functions (eg. TreeBuilder) recursively call update methods on collection objects. Allowing recursing edit calls with previews is non-trivial as it introduces performance concerns and ambiguities over the order of preview events, the state of the datastructure at each point and more. This pull request handles it as follows. If a collection is updated during the invocation of another update on the same collection, then its changes are merged with the main update. The change is immediately visible inside the update action as is expected, but the preview/change event are not triggered until when the topmost update function exits. This ensures that the collection state is consistent in the update function. It also improves performance by having each nested update operate on the same collection instance (instead of creating an new copy each time) and merging the different changesets into one notification. Finally, it also ensures that both Preview() and Connect() receive the same changesets, in the same order.
  • Editing the collection in a Preview() subscriber is not allowed, as this would produce unexpected behavior. This is very rarely needed anyway.

A note about locks:

  • During development I first replaced the locks inside the ReaderWriters with ReaderWriterLockSlim. It turns out this was not really necessary for the implementation of the Preview() 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 of ReaderWriterLockSlim I found that some of the tests for TreeBuilder were failing. This is because TreeBuilder 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 the ReaderWriterLockSlim 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 used TwoStageRWLock instead of ReaderWriterLockSlim. 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.

@RolandPheasant
Copy link
Collaborator

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

@RolandPheasant
Copy link
Collaborator

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.

@Wouterdek
Copy link
Contributor Author

Here is a more concrete example of where Preview() could be useful:

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:

Company
   -> ISourceList<Department> Departments;
   -> ISourceList<Relationship> Relationships;

Relationship
   -> Person A;
   -> Person B;

Department
   -> Company Parent;
   -> ISourceList<Person> People;

Person
   -> Department Parent;
   -> IObservableList<Relationship> Relationships; // Contains all company relationships that involve this person.

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:

department.People.Remove(person);
   => Using People.Connect().ActOnEveryObject: person.Department is set to null;
      => person notices Department is set to null: person.Relationships gets cleared
   => Using People.Connect().OnItemRemoved: network.Relations.RemoveAll(person.Relationships);
      => Doesn't work as expected, because the relationships were already cleared

Solution:

department.People.Remove(person);
   => Using People.Preview().OnItemRemoved: network.Relationships.RemoveAll(person.Relationships);
   => Using People.ActOnEveryObject: person.Department = null;
      => person notices Department is set to null: person.Relationships gets cleared.

@Wouterdek
Copy link
Contributor Author

Wouterdek commented Feb 16, 2019

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?

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().

@RolandPheasant
Copy link
Collaborator

RolandPheasant commented Feb 25, 2019

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

@Wouterdek
Copy link
Contributor Author

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.

@Wouterdek Wouterdek mentioned this pull request Mar 14, 2019
@Wouterdek Wouterdek closed this Mar 30, 2019
@lock lock bot locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants