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

Isolate the GraphClient in another thread #424

Open
Sewdn opened this issue Sep 28, 2019 · 27 comments
Open

Isolate the GraphClient in another thread #424

Sewdn opened this issue Sep 28, 2019 · 27 comments
Assignees
Labels
dart client Issue relates mainly to the standalone dart client enhancement New feature or request help wanted Extra attention is needed performance Priority: Medium Medium priority to include it inside the next release PRs encouraged PRs are welcome if stakeholders want to take ownership
Projects
Milestone

Comments

@Sewdn
Copy link

Sewdn commented Sep 28, 2019

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 mainawycliffe added enhancement New feature or request help wanted Extra attention is needed labels Sep 29, 2019
@mainawycliffe
Copy link
Collaborator

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
Copy link
Author

Sewdn commented Oct 1, 2019

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
Copy link
Collaborator

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.

@micimize micimize added the PRs encouraged PRs are welcome if stakeholders want to take ownership label Nov 7, 2020
@msal4
Copy link

msal4 commented Apr 18, 2021

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

@micimize
Copy link
Collaborator

@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:

  • serialization for options and results
  • to create an "isolate wrapper" that:
    • reads from the isolate stream and calls the appropriate method+options combo
    • send the results back (you might need message ids or something for making sure the results get to the right place)
  • to create an "isolate client" that proxies calls to the provided isolate
  • for subscribe, you'll need to parse a message for the id, and then filter on the isolate stream for messages with that id, then return that filtered stream.

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
Copy link

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
Copy link
Collaborator

budde377 commented Jul 8, 2022

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
Copy link

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

@vincenzopalazzo vincenzopalazzo added this to To do in v5.2.0 via automation Jul 8, 2022
@vincenzopalazzo vincenzopalazzo added this to the v5.2.0 milestone Jul 8, 2022
@vincenzopalazzo vincenzopalazzo added dart client Issue relates mainly to the standalone dart client Priority: Medium Medium priority to include it inside the next release labels Jul 8, 2022
@vincenzopalazzo
Copy link
Collaborator

Thanks to resurrect this feature :)

@insidewhy
Copy link

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
Copy link

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

@insidewhy
Copy link

@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
Copy link

aquadesk commented Aug 7, 2022

I am looking forward to testing this fix.
I think this could fix my issue here #1187

@daksh-gargas
Copy link

daksh-gargas commented Aug 10, 2022

@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
Copy link
Contributor

knaeckeKami commented Oct 30, 2022

@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
Copy link

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

@orestesgaolin
Copy link

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
Copy link

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
Copy link

biklas7 commented Feb 3, 2023

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
Copy link

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
Copy link
Contributor

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:
#1196 (comment)
dart-lang/collection#263 (comment)

@vincenzopalazzo
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link

Unfortunately my app is being impacted by this issue also.

@RobertBrunhage
Copy link

Just for transparency, I implemented the approach mentioned by @knaeckeKami, and here is the result #1315 (comment)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart client Issue relates mainly to the standalone dart client enhancement New feature or request help wanted Extra attention is needed performance Priority: Medium Medium priority to include it inside the next release PRs encouraged PRs are welcome if stakeholders want to take ownership
Projects
v5.2.0
To do
Development

No branches or pull requests