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

Miscellaneous cleanup #3

Open
nickynick opened this issue Jan 22, 2014 · 12 comments
Open

Miscellaneous cleanup #3

nickynick opened this issue Jan 22, 2014 · 12 comments

Comments

@nickynick
Copy link
Contributor

Hi!

First of all, I wanna say thanks for creating and sharing this cool stuff, I'm looking forward to using it in my projects. And I feel like there are a couple things that could be done to make it even better :)

Here are the points I wanna ask you about:

  • Wouldn't it be better to use parent-child context hierarchy for background operations? I believe that this newer way is generally more preferable and convenient than doing merge with didSave notifications, because it doesn't require any additional setup. rcd_merged signal can be replaced with a signal for NSManagedObjectContextObjectsDidChangeNotification to serve the similar purpose.
  • This point kind of follows up the previous one. It feels like the methods + (NSManagedObjectContext *)context and + (NSManagedObjectContext *)currentContext in the NSManagedObjectContext (ReactiveCoreData) category are a bit confusing... Perhaps we could just eliminate the former one?
  • The methods - (RACSignal *)findAll: and - (RACSignal *)findOne: in RACSignal (ReactiveCoreData) category ignore the receiver completely and just return new signals... I guess they should be class methods instead?
  • Would it make sense to move methods + (instancetype)insert and + (instancetype)insert: from NSManagedObject (ReactiveCoreData) to some other place or maybe remove them completely? They seem like they have nothing to do with ReactiveCocoa stuff :)

Let me know what do you think about this. If you are okay with these points, I'm willing to contribute :)

@apparentsoft
Copy link
Owner

Regarding parent-child contexts. I've found and also read in other blogs that parent-child context do not perform better. They may be easier to set up but have (or at least had) subtle bugs that requires work-arounds. Performance-wise, they are actually slower, if I remember correctly.

One consideration: the child context, when in background queue, doesn't actually save to the persistent store when - save: is issued. It merges data into its parent. Thus, saving is either left to the main thread (bad) or you need to set up background context, which is a parent of the main threads's context (grand parent of the child). So you need to perform another save from main to push to its parent and then do a save there to actually push data to disk. This isn't faster and prone to problems and other issues. Basically, it hides a lot of implementation from you and you need to know really well what happens in all the cases.

For example, if you change object's attributes in the main thread's context after child is created and you perform a fetch in child, do you see the changes in the modified object? Does it depend on whether it's already pushed to main thread's parent? Does it depend on if it's already saved to disk? Do you know answers to all these?

I'm using this (parent-child) architecture in Trickster, one of my application. I've had a lot of problems with this. It kinda works now but sometimes it may get an occasional crash when several heavy operations are performed in the background. It's hard to debug this because some of these aspects are not well documented or do not perform according to the documentation.

The old method of saving in the background and then merging changes manually is much more straightforward, debugged, and better understood.

For reference about the performance of different setups read the following three blog posts:
http://floriankugler.com/blog/2013/4/2/the-concurrent-core-data-stack
http://floriankugler.com/blog/2013/4/29/concurrent-core-data-stack-performance-shootout
http://floriankugler.com/blog/2013/5/11/backstage-with-nested-managed-object-contexts

@apparentsoft
Copy link
Owner

-currentContext and context have different functions.
The former returns the context associated with current scheduler. The latter creates a new child context from the current one.

@apparentsoft
Copy link
Owner

Regarding findAll and findOne. There are actually already two of them. The class methods in NSManagedObject (ReactiveCoreData) and instance methods in RACSignal (ReactiveCoreData). Usually the class methods are more useful and they start the signal chain. The idea for RACSignal instance methods is for a case when you want to start a fetch as a result of a "next" from another signal. For example from a trigger signal. Not sure how useful this is but I had this idea and thought it's useful, so implemented it.

@apparentsoft
Copy link
Owner

The insert method indeed doesn't have anything to do with ReactiveCocoa itself but it's a good shorthand. It integrates with RCD for getting current context.

+ (instancetype)insert;
{
    return [NSEntityDescription insertNewObjectForEntityForName:[self entityName] 
             inManagedObjectContext:[NSManagedObjectContext currentContext]];
}

@nickynick
Copy link
Contributor Author

@apparentsoft Thanks a lot for the detailed replies!

Regarding parent-child contexts... Well, yeah, I was thinking about it a bit more and became pretty unsure about it myself. I'm using this approach in some of my applications and it generally seems to work fine (save for one nasty bug in iOS 5) and simplifies the code a bit. However, I see potential problems with using it together with RAC because of requirement to perform operations in private queues. Your points are also valid. So yeah, I guess it's better like it is now.

-currentContext and context have different functions.
The former returns the context associated with current scheduler. The latter creates a new child context from the current one.

That's right. I was trying to say that the name of the latter method may be a bit confusing to some people.

The idea for RACSignal instance methods is for a case when you want to start a fetch as a result of a "next" from another signal. For example from a trigger signal. Not sure how useful this is but I had this idea and thought it's useful, so implemented it.

Sorry because I'm still pretty new to ReactiveCocoa, but will this actually work? It seems to me that the receiver signal will just be lost.

@apparentsoft
Copy link
Owner

I was trying to say that the name of the latter method may be a bit confusing to some people.

You might be right and context might be a confusing name. Perhaps newChildContext is a better name.

It seems to me that the receiver signal will just be lost.

As I understand it, not the signal will be lost, but what the signal sends next. But consider that the signal is just a trigger and you don't care about its value, or you check its value in another reactive chain.

In practice, I've not yet used this method. I think it'll work. Perhaps it's a bad approach and - (RACSignal *)fetchWithTrigger: replaces it.

@nickynick
Copy link
Contributor Author

I just took an example for findAll: from the spec and modified it a bit like this:

[[[[RACSignal return:@1] doNext:^(id x) {
    NSLog(@"hi!");
}] findAll:Parent.entityName] subscribeNext:^(id x){
    completed = YES;
    actual = x;
}];

And NSLog is actually never called. It's because nothing is done with that signal, nobody is actually subscribed to it, findAll: just returns an entirely new signal.

@apparentsoft
Copy link
Owner

I see your point (and my bug).
Maybe it should be something like this instead:

- (RACSignal *)findAll:(NSString *)entityName;
{
    NSFetchRequest *fetchRequest = [NSFetchRequest fetchRequestWithEntityName:entityName];
    return [self mapReplace:fetchRequest];
}

@nickynick
Copy link
Contributor Author

Yeah, this one should work. I'm not sure if it's worth its money, though. As you pointed out, -fetchWithTrigger: seems like a better and more obvious way of achieving this behaviour.

@apparentsoft
Copy link
Owner

Maybe it's better to just remove these two methods from the code, then.

@nickynick
Copy link
Contributor Author

Let's do it! And also the context method renaming part. Actually, I feel like I want to rename - (NSManagedObjectContext *)mainContext; and + (NSManagedObjectContext *)contextWithMainContext:(NSManagedObjectContext *)mainContext; too because the term "main" usually refers to the main thread, which is pretty confusing. Perhaps "parent" is a better word?

@apparentsoft
Copy link
Owner

Well, the idea here is that the main context is really the main context. Yes, it's also the parent, but it should be the real main context. - (void)rcd_mergeChanges:(NSNotification *)note; has the following code, which uses this assumption (notice the thread):

[mainContext performSelector:@selector(mergeChangesFromContextDidSaveNotification:) 
  onThread:[NSThread mainThread] 
  withObject:note 
  waitUntilDone:YES];

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

No branches or pull requests

2 participants