zino-hofmann / graphql-flutter

A GraphQL client for Flutter, bringing all the features from a modern GraphQL client to one easy to use package.
https://zino-hofmann.github.io/graphql-flutter
MIT License
3.25k stars 620 forks source link

Isolate the GraphClient in another thread #424

Open Sewdn opened 5 years ago

Sewdn commented 5 years ago

Is your feature request related to a problem? Please describe. The Main thread of a flutter app, should be reserved for UI, to keep the rendering and interactions as smooth as possible. Currently all computation by the GraphQL client is run in the same thread, competing with the UI to get CPU time for fetching and parsing data, normalizing it, encoding it to json and storing it to cache. This results in jittered UI when lots is taking place

Describe the solution you'd like Running the entire GraphlClient and Cache (and persistence backend) in a separate Isolate (or multiple Isolates), would result in more performant flutter apps, keeping more CPU time available for the main thread (UI). While still offering the same api for querying and mutating data, it would abstract away the complexity of sending and receiving messages from other thread.

Describe alternatives you've considered When trying to persist cache more often then only during app shutdowns, we stumbled on rendering issues, because encoding the entire cache to json before persisting it, can be very CPU consuming. We tried to offload only cache persistence to a seperate Isolate, but since no memory is shared between 2 threads, and only primitives are allowed as arguments when sending messages to another thread, we didn't end up with a good solution. It makes more sense to shift the entire apollo client to another thread, because the graphql client's api is suited for sending these requests to another thread (sending operations and variables).

Additional context

423

mainawycliffe commented 5 years ago

Yeah, this looks like a good idea. If we go down this path, we could make it configurable. I am just wondering if flutter for web supports isolates.

Sewdn commented 5 years ago

I can imagine Isolates could work for web, making use of webworkers. Don't know if Isolates are already been adapted to making use of them for web... I'll look into it.

mainawycliffe commented 5 years ago

That would be the ideal way, for now, it seems, they have not been adapted to be used by flutter for web and would require a platform check before using them to make sure we support flutter for web.

msal4 commented 3 years ago

I'm interested in working on this one, any tips on where I should start? @mainawycliffe @micimize

micimize commented 3 years ago

@msal4 if you aren't already have familiar with Isolates, take a look at Isolate 2-Way Communication and other articles. In terms of implementation guidance (from someone who knows very little about isolates), my first recommendation would be to not put the whole client in an isolate, and first create a link isolate.

Creating a link isolate: Because gql_links are stream-based already, you'd just have to wrap a given the given link in an isolate, serializing requests into the isolates and parsing responses. You'll also need to provide request parsing and response serialization on the other side of the isolate.

Later if this is a successful approach you could contribute some optimizations to other links that avoid the need for the doubling of serialization/parsing.

You can't really create a cache isolate because the cache has a synchronous API, and this would probably not be worth all the heavy message passing anyhow.

Creating a client isolate: The client itself has a lot of hard-to-serialize api surface area. I'd recommend ignoring watchQuery / watchMutation initially, and focusing on query, mutate, and subscribe. You'd need:

For watchQuery / watchMutation, you need to deal with the fact that they return an entire ObservableQuery, for which part of the API is setting functional callbacks, and which is fairly coupled to the QueryManager. You could have a proxy-ing version of ObservableQuery, but this part will be very tough, because for instance, Mutation's update takes in a cache proxy, so then you have to proxy through that to the isolated proxy. hive probably still can't be used in multiple isolates, so you probably can't have some trick where we have a partial client in the main thread.

And the last thing I'll say is that I'm actually fairly skeptical of this yielding performance gains. We have a much better caching solution than when this was initially opened, and isolating the client adds a fair amount of indirection for what is essentially a high-throughput, highly-coupled slice of functionality. I don't know much about dart isolates though, so I'd do some research into exactly how this will change the profiling of an app before embarking. This is what makes starting with a simple link such a good idea – minimum scenario for testing whether this is worth doing and what performance gains can be achieved.

daksh-gargas commented 2 years ago

It's so weird how the package still is performing all the tasks on the Main thread. I mean that's just basic practice to not do such computation-intensive tasks on the main thread.

budde377 commented 2 years ago

It's so weird how the package still is performing all the tasks on the Main thread. I mean that's just basic practice to not do such computation-intensive tasks on the main thread.

@daksh-gargas no one is stopping you from submitting a pr

daksh-gargas commented 2 years ago

Working on it rn... will probably add one soon @budde377 :)

vincenzopalazzo commented 2 years ago

Thanks to resurrect this feature :)

insidewhy commented 2 years ago

We're working on a product where even a few requests causes the whole app to lag since the CPU of the device isn't very powerful.

daksh-gargas commented 2 years ago

@insidewhy I am almost done with a potential fix to this problem. Just testing it out. Will update soon!

insidewhy commented 2 years ago

@daksh-gargas Okay let me know if you need some help testing, I can try your branch against our product when it's ready if you want a second pair of eyes.

aquadesk commented 2 years ago

I am looking forward to testing this fix. I think this could fix my issue here https://github.com/zino-hofmann/graphql-flutter/issues/1187

daksh-gargas commented 2 years ago

@aquadesk Wrap your whole GraphQL service inside IsolatedBloc or Isolates

I'll post the full working solution and reasoning for this approach real soon, but for the time being, please use that to unblock yourself! :)

knaeckeKami commented 1 year ago

@vincenzopalazzo If you're interested, I recently implemented support for running the client in an Isolate here:

https://github.com/gql-dart/ferry/blob/master/packages/ferry/lib/src/isolate/ https://github.com/gql-dart/ferry/blob/master/packages/ferry/lib/ferry_isolate.dart Documentation: https://ferrygraphql.com/docs/isolates

I'm not 100% satisfied with the solution yet, though, as custom communication between the isolates (for example when deleting the auth token on logout) still requires users to understand Isolates and SendPorts to some degree and I would like to improve that in future versions.

insidewhy commented 1 year ago

@daksh-gargas I don't suppose you had any success?

orestesgaolin commented 1 year ago

Alternatively, instead of keeping the whole client in a separate isolate, you can just delegate the json parsing via compute method. I've posted a sample implementation here and it's been working fine for the last month on production with several hundred users. One caveat is that if you have few queries being processed/deserialized and you do the hot restart in VS Code, it may lead to unhandled exceptions related to isolates, but it's not happening in release builds.

daksh-gargas commented 1 year ago

Seems like you didn't account for processing/parsing of objects inside GQL Cache That also happens on the main thread unless moved to an isolate :)

Alternatively, instead of keeping the whole client in a separate isolate, you can just delegate the json parsing via compute method. I've posted a sample implementation here and it's been working fine for the last month on production with several hundred users. One caveat is that if you have few queries being processed/deserialized and you do the hot restart in VS Code, it may lead to unhandled exceptions related to isolates, but it's not happening in release builds.

biklas7 commented 1 year ago

Was there any development on this topic? I'm also developing an app that relies heavily on GQL Cache and I still feel the performance hit, even though the app isn't that big. Should I look into other alternatives myself for now?

insidewhy commented 1 year ago

Was there any development on this topic? I'm also developing an app that relies heavily on GQL Cache and I still feel the performance hit, even though the app isn't that big. Should I look into other alternatives myself for now?

I think you'll need to write your own PR to fix this or find an alternative, with our simple app the performance is far too bad on anything but high-end phones. We don't consider this library usable in its current state.

knaeckeKami commented 1 year ago

FWIW a big performance boost can be achieved by replacing calls to DeepCollectionEquality.equals() with a custom json equals method like this:

/// compare two json-like objects for equality
/// parameters must be either null, num, bool, String or a List or Map of these types
/// this is an alternative too DeepCollectionEquality().equals, which is very slow and has
/// O(n^2) complexity:
bool jsonMapEquals(Object? a, Object? b) {
  if (identical(a, b)) {
    return true;
  }
  if (a == b) {
    return true;
  }
  if (a is Map) {
    if (b is! Map) {
      return false;
    }
    if (a.length != b.length) return false;
    for (var key in a.keys) {
      if (!b.containsKey(key)) return false;
      if (!jsonMapEquals(a[key], b[key])) return false;
    }
    return true;
  }
  if (a is List) {
    if (b is! List) {
      return false;
    }
    final length = a.length;
    if (length != b.length) return false;
    for (var i = 0; i < length; i++) {
      if (!jsonMapEquals(a[i], b[i])) return false;
    }
    return true;
  }
  return false;
}

Doing so would likely make this fast enough to this is not an issue any more for many apps. And it should be much much easier do just switch the equals() implementation than to migrate to isolates.

See also: https://github.com/zino-hofmann/graphql-flutter/issues/1196#issuecomment-1370730735 https://github.com/dart-lang/core/issues/645

vincenzopalazzo commented 1 year ago

Yeah, this is the solution @knaeckeKami moving this in a new isolate will hide just the problem.

Like if my machine is too slow, I buy a bigger one instead to optimize the software that I use!

I will assing this to my self, and see we can include this inside the 5.2

insidewhy commented 1 year ago

moving this in a new isolate will hide just the problem.

All significant processing should be moved into an isolate though, other processing that graphql-flutter is doing is also fairly intensive and has the chance to lag the UI. So it's not only hiding the problem but also a part of best practice, ideally both of these solutions should be applied.

vincenzopalazzo commented 1 year ago

we start with the first one and then we see if the isolate is necessary! if you want make this test, feel free to open a PR

ayyoubelamrani4 commented 1 year ago

we start with the first one and then we see if the isolate is necessary! if you want make this test, feel free to open a PR

There are options to avoid the first one, but even with that, having the main thread taking care of requests and communicating with the cache etc ... is very heavy and might create lots of jitters depending on the app.

Is anyone working on having isolates for this lib?

mattsrobot commented 1 year ago

Unfortunately my app is being impacted by this issue also.

RobertBrunhage commented 7 months ago

Just for transparency, I implemented the approach mentioned by @knaeckeKami, and here is the result https://github.com/zino-hofmann/graphql-flutter/issues/1315#issuecomment-1963470337

In short, I feel like the equality checks should be improved as a high priority