Refactor RealmQuery to allow async queries

[nhachicha]

Now that we have a POC of a working async queries with a convergence strategy https://github.com/realm/realm-java/pull/1214 I suggest the following refactor to enable RealmQuery to be used seamlessly for both sync & async queries.

Unless we define a DSL that represent a query we can push (in one call) from Java to Core. We need to evaluate lazily the query once we call findAll, findFirst etc. Instead of calling Core for each step (e.g. greaterThan, isNotNull). As suggested by @cmelchior we should approach it as a command pattern, basically queue all calls to native in a list of Runnable, then call execute to perform each call. at this point we can overload execute to run on a background thread (with a callback)

the result will be like this

// Build the query looking at all users:
RealmQuery<User> query = realm.where(User.class);

// Add query conditions:
query.equalTo("name", "John");
query.or().equalTo("name", "Peter");

// Execute the query in the caller thread
RealmResults<User> result1 = query.findAll();

// Execute the query on a worker thread:
RealmQuery.AsyncRequest result1 = query.findAllAsync(new RealmQuery.Callback() {
  public void onSuccess(RealmResults<Foo> results) {
    // Use results
 }
});

realm.where(Foo.class).findFirstAsync(new RealmQuery.Callback() {
  public void onSuccess(Foo result) {
    // Use result
 }
});

// User can cancel the pending query
result1.cancel();

WDYT? @bmunkholm @emanuelez @kneth

[cmelchior]

There is two things in this IMO. Which is two separate discussions.

1) User facing API 2) Internal implementation.

1: User API I think this proposal look great. Some comments:

• We need to handle errors, especially invalid queries in a nice way. Currently I don't see any ways around registering an error handler somehow? I like that the callback only has one return method, as it reduces the boilerplate you have to look at. Perhaps having a separate callback for errors that people can use if they wish, just like RxJava OnError? We might want to do something special about Java errors so users doesn't accidentally ignore those?

realm.where(Foo.class).findAllAsync(onSuccess)``` // This would throw an exception if it fails
realm.where(Foo.class).findAllAsync(onSuccess, onError)``` // Exception is routed to onError instead. 

• Nitpick as naming looks fine, but not sure if RealmQuery.asyncFindAll() is better than RealmQuery.findAllAsync()?

2: Internal implementtion A Java command pattern where each command map to a Core method call would probably be fairly easy to implement. I am not sure we need to wrap each in a runnable, that should be done at execution time for all of them? However a internal DSL has the advantage that executing that DSL can be moved to the C++ Object Store which could benefit the other bindings as well. I would assume they will be facing the same challenges as we do now. Also having to call JNI once also has obvious speed advantages, although that is not as important for creating a queries as it doesn't happen often. We should probably discuss this with Cocoa/Swift as well?

[nhachicha]

1: User API I forget to past the onError call Although I do like the RxJava like syntax with

realm.where(Foo.class).findAllAsync(onSuccess)``` // This would throw an exception if it fails
realm.where(Foo.class).findAllAsync(onSuccess, onError)``` // Exception is routed to onError instead. 

I do prefer findAllAsync as its a bit consistent with findAll. what about another option where we overload findAll only? If the user provide a RealmQuery.Callback then this means the call must happen in a worker thread?

realm.where(Foo.class).findAll(new RealmQuery.Callback() {
  public void onSuccess(RealmResults<Foo> results) {
    // Use results
 }
});

2: Internal implementtion We can have a 1:1 mapping between methods ex: greaterThan & it's arguments or just wrap the existing call inside a Runnable. Example:

query.or().equalTo("name", "Peter");

will result in

List<Runnable> cmds = ...
//...
cmds.add(new Runnable(){
      this.query.or();
});
cmds.add(new Runnable(){
        this.equalTo(fieldName, value, CASE_SENSITIVE)
});
// inside findAll
for (Runnable step:cmds) {
      step.run();
}

[tgoyne]

What's the motivation for building up a command list and creating the actual Query object on the target thread? It's not required for the public API you've listed and even with JNI overhead it seems very unlikely that it'd be a meaningful reduction in the time spent on the source thread.

[nhachicha]

This is basically changing the evaluation strategy & delaying all the calls you would have to do within the caller's thread to a worker thread (JNI overhead is the same, but done on a background thread)

The command list is not introducing a new API, but an example of how we can keep the same interface but internally change the behaviour.

[tgoyne]

What is the benefit of delaying it? Are there scenarios where constructing the Query object actually takes a meaningful amount of time? Benchmarking the obj-c query code has consistently shown the NSPredicate stuff taking significantly longer than building the Query object, and the two combined taking an entirely inconsequential amount of time outside of extreme scenarios (such as an IN clause on tens of thousands of items). Obviously the JNI adds some overhead, but something like realm.where(User.class).equalTo("name", "John").or().equalTo("name", "Peter") should only require four native calls.

[emanuelez]

It's not really about performance, but about being able to do something like:

realm.where(Dog.class).equalTo("age", 3).findAllAsync(YourCallbackHere);

as opposed to

realm.whereAsync(Dog.class, YourCallbackHere).equalTo("age", 3).findAll();

which would be pretty ugly (at least in Java land).

[tgoyne]

Why is it needed for that? Given the ability to pass Query objects between SharedGroups (which the handover PR claims to support) the first seems completely straightforward to implement with the current approach of building up the Query object directly.

[nhachicha]

Actually what we handover is not the Query but the TableView result exported from the the background thread/shared_group to the caller thread.

Even if we assume that the time to build the query is significantly smaller than running the predicate, the build up still happen on the caller thread (which is most of the time the UI thread on Android), this will block the 16ms VSYNC signals.

[cmelchior]

I actually think @tgoyne has a point. Until we actually execute `.findAll() we only have a query object, and if we can transfer that from the UI thread to the worker thread before executing the .findAll method call, we have what we want. Returning we obviously want to transfer the tableview though.

[nhachicha]

I'm fine with this approach if we can guarantee that building the query will not exceed 16ms I still argue that we're doing multiple JNI call on UI thread

[tgoyne]

Before thinking about doing something for the sake of performance you should start with measuring the performance of the simplest thing. If a handful of JNI calls on the main thread is actually a problem then I don't see how handover could be useful at all, as doing anything with the results on the main thread will involve a lot more JNI calls.

Outside of the JNI overhead, it should not be a problem at all. In obj-c constructing a moderately complicated query ("boolCol = false and (intCol = 5 or floatCol = 1.0) and objectCol = nil and longCol != 7 and stringCol IN {'a', 'b', 'c'}") on the lowest end iOS device we support (an ipod touch) takes .3ms. Supported Android devices probably go quite a bit lower-end than that, but not an order of magnitude slower/

[nhachicha]

Again nobody is talking about performance, but reducing the number of calls from UI thread. we need one call to import_from_handover from the UI as opposed to build the entire query.

I'll try your suggestion & measure the outputs Cheers,

[tgoyne]

The number of JNI calls from the UI thread and performance are not distinct things. The performance implications are the only reason to even think about the number of JNI calls.

[bmunkholm]

I don't think we need to worry about a few JNI calls on the UI thread when building the query object. Right now we even run the query on the UI thread without any problems - unless it's huge queries.

[OlegDokuka]

May be it`s better to do just RxJava api and return Observable if user need? I think it is better then create own realization of this. Guys from Square made it with their Retrofit

[cmelchior]

Yes, expect that would introduce a dependency on RxJava, which not all would want to drag into their project as it is both complicated and quite heavyweight.

We have an issue for explicit RxJava support ( #865 ), where something like returning an observable instead would be a perfect fit. This would probably be an add-on just like RxAndroid is to RxJava, but we haven't decided fully on that yet.

[IgorGanapolsky]

Any update on integrating RxJava into this feature? I'd love to use it with Realm in my project.

[nhachicha]

Hi @IgorGanapolsky as a first step, we're about to release the support for async queries/transactions using a worker thread. https://github.com/realm/realm-java/pull/1214

we still have some of the constraints mentioned here https://github.com/realm/realm-java/issues/865 (basically because subscription can occurs on a different threads, we are not fully supporting RxJava, since you can't pass RealmObject/RealmResults between threads)

we will naturally support RxJava once we allow RealmObject/RealmResults to be used from different threads, this require some change to our storage engine.

[nhachicha]

please follow progress here #1214