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

Use Isolates (compute()) for JSON De-serialization #60

Closed
awanishraj opened this issue Sep 27, 2019 · 30 comments
Closed

Use Isolates (compute()) for JSON De-serialization #60

awanishraj opened this issue Sep 27, 2019 · 30 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers wontfix This will not be worked on
Milestone

Comments

@awanishraj
Copy link

The library currently is causing jank due to deserialization on the main thread. This can be pushed to an isolate without changing the library's apis as mentioned here - https://flutter.dev/docs/cookbook/networking/background-parsing

@trevorwang trevorwang added enhancement New feature or request good first issue Good for newcomers labels Sep 30, 2019
@trevorwang trevorwang self-assigned this Sep 30, 2019
@trevorwang
Copy link
Owner

@awanishraj

Please have a try https://github.com/flutterchina/dio_flutter_transformer

@stale
Copy link

stale bot commented Nov 15, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 15, 2019
@stale stale bot closed this as completed Nov 22, 2019
@fryette
Copy link

fryette commented Dec 11, 2019

@trevorwang but library generates code, which runs in main thread!
As example

@override
  sites(vanId, start, length) async {
    ArgumentError.checkNotNull(start, 'start');
    ArgumentError.checkNotNull(length, 'length');
    const _extra = <String, dynamic>{};
    final queryParameters = <String, dynamic>{
      'start': start,
      'length': length
    };
    final _data = <String, dynamic>{};
    final Response<List<dynamic>> _result = await _dio.request('/Site',
        queryParameters: queryParameters,
        options: RequestOptions(
            method: 'GET',
            headers: <String, dynamic>{},
            extra: _extra,
            baseUrl: baseUrl),
        data: _data);
    var value = _result.data
        .map((dynamic i) => SiteModel.fromJson(i as Map<String, dynamic>))
        .toList();
    return Future.value(value);

Okay, we see that request could be in separate process, but other code, runs in main thread and cause UI freezes

@nioncode
Copy link

nioncode commented Jan 12, 2020

This is critical and leads to serious ui jank as soon as the returned json gets too big.
The solution is pretty simple:

  1. Wrap everything related to converting the response to the result type in a static method on the generated class with the following signature:
static <ResultType> _ResultType_fromResponse(Map<String, dynamic> json) {
// Do whatever is necessary to convert the response
return ResultType.fromJson(json);
}
  1. After awaiting the response, instead of directly converting the response to the target type, simply call the static method from above wrapped in compute:
final Response<Map<String, dynamic>> _result = await _dio.request(
    '/apicall',
    queryParameters: queryParameters,
    options: RequestOptions(
        method: 'GET',
        headers: <String, dynamic>{'authorization': authorization},
        extra: _extra,
        baseUrl: baseUrl),
    data: _data);
final value = await compute(_ResultType_fromResponse, _result.data);
return Future.value(value);

The only problem with this is that compute is part of the flutter sdk and not part of dart itself. To combat this, we can write our own compute method that simply uses Isolate.spawn under the hood.
I tested this by manually modifying the generated code, but unfortunately I don't know how to modify the generator to emit the additional static methods.

Launching a new isolate takes ~25ms on a Pixel 2 in Profile Mode. This delay can be completely removed by instantiating the isolate on the first request and caching it afterwards. However, users of the generated class would then have to dispose() the class to clean up the cached isolate. For now, I guess the 25ms hit is acceptable, since a network request usually takes longer anyways, but we might want to keep this in mind as a future optimization.

EDIT: it probably makes sense to also move the transformation from String to Map<String, dynamic> into the _ResultType_fromResponse method to avoid freezing the UI while decoding the json, similar to https://github.com/flutterchina/dio_flutter_transformer, but running both in the same isolate.

EDIT: while working on a PR for this, I noticed that we might also want to do this when encoding objects to not freeze the UI when big objects are being sent.

@nioncode
Copy link

I created a pull request with a first draft for this in #94, any feedback would be great.

@trevorwang trevorwang added this to the v1.1 milestone Feb 1, 2020
@trevorwang trevorwang removed the wontfix This will not be worked on label Feb 1, 2020
@trevorwang trevorwang reopened this Feb 1, 2020
@trevorwang trevorwang modified the milestones: v1.1, v1.3 Feb 7, 2020
@stale
Copy link

stale bot commented Feb 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Feb 22, 2020
@stale stale bot closed this as completed Feb 29, 2020
@parkgrrr
Copy link

Are there any plans to address or reopen this issue?

@giaur500
Copy link

giaur500 commented Sep 1, 2020

I guess not. So Retrofit should be avoided with Flutter, because it's useless here.

@frencojobs
Copy link

This issue is the only thing that's stopping me from using retrofit. Even if there's no solution currently, it would be nice to have a workaround to bypass this issue temporarily.

@trevorwang trevorwang reopened this Sep 25, 2020
@stale stale bot removed the wontfix This will not be worked on label Sep 25, 2020
@giaur500
Copy link

Yes, my case is the same. Dev should reconsider to add support for it. Otherwise, make it not compatible with Flutter, as it useless with Flutter. Note that 90% of Dart development is Flutter, so that's not wise to drop Flutter support.

@awanishraj
Copy link
Author

@giaur500 It really is not as big of a deal as you are making it out to be for most developers. It's a nice to have, but not the make or break. I tried with isolates myself and found that it makes all api calls much slower. So, I would suggest you do some experimentation by yourself. The lib might not suit you, but it's definitely useful for majority of flutter developers. No need to give the dev a hard time on this.

@trevorwang
Copy link
Owner

@giaur500 I really agree with @awanishraj .

Currently, we don't have a nice workaround for this issue. And I would suggest that have a try in another way to check whether it really caused a performance issue.

I was considering a big refactor for this library, but I don't have much time these days. Trying to cover this issue in the next big release.

@giaur500
Copy link

giaur500 commented Sep 25, 2020

Do you mean compute/isolates is does not make any noticeable difference? That's recommended way to json deserialize in Flutter. Of course, I can also don't care about that and deserialize json in main thread, but that's not recommended, as on slow device it may cause lags.

Decreasing user experience to get more elegant code should be always avoided in my opinion. From other side, as I am coming from Android (native), lack of Retrofit equivalent is painffull for me.

@frencojobs
Copy link

frencojobs commented Sep 25, 2020

The early commits of this project https://github.com/frencojobs/scrapbook can be seen to be using retrofit. But since the API I was consuming was too large and not paginated, the UI lag/jank gets super annoying on my low-end device. If I am not mistaken, the jank lasts maybe up to 10s, which is a lot. So, I rewrote all the API codes as plain functions and made it utilize the compute function, and the results are far superior. Isolates may be making all the API calls a bit slower, but there won't be at least an annoying UI jank.

But I couldn't rush an opensource project which I am using for free. Everyone's having a hard time currently. I could understand that. I'm just putting two cents in for a project I really love.

  • Use isolates/compute please, they really do help according to my experience
  • Btw, the generated code is not formatted according to pedantic, so it's causing lots of warnings

Looking forward to the big release : )

@Abdktefane
Copy link

any new with this issue ?

@giaur500
Copy link

Can you see any new comments from devs? If not, no news. No need to ask.

@Abdktefane
Copy link

@giaur500 A new question for the issue prevents the bot from closing it and gives the issue importance to the author. Is it necessary to be rude? If yes then you can ignore the question. If no, you are welcome to answer

@giaur500
Copy link

giaur500 commented Oct 16, 2020

Ok fine. But I think author knows about importance, but still it has low priority. Simply, don't use Retrofit, as it's useless with Flutter. Almost one year without even any idea how to fix it.

As I understand, this project is not intended to be used with Flutter at the first stage, it's rather for Dart, so I can understand reason why this issue is not going to be fixed in near future. It works with Flutter, but not written for Flutter

@parkgrrr
Copy link

parkgrrr commented Nov 3, 2020

Would the dio flutter transformer work with retrofit to serialize in an isolate?

https://github.com/flutterchina/dio_flutter_transformer

@frencojobs
Copy link

No. The library still generates code to serialize the response, which runs in the main thread. Something like this,

final value = ResponseModel<OtpModel, Null>.fromJson(_result.data);
// ☝️ this still runs in the main thread
return value;

@trevorwang
Copy link
Owner

Yes, exactly. Currently, I haven't found a way to use one method to handle the type converting due to generic types.

Any ideas or PRs are appreciated

@stale
Copy link

stale bot commented Dec 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 4, 2020
@stale stale bot closed this as completed Dec 13, 2020
@livotov
Copy link

livotov commented May 6, 2021

I think retrofit is doing smth weird - just got the same problem with the long API response - huge JSON array, 4500 items, 20 to 50 fields each.

final MyResponseObject res = await myRetrofit.fetchData() takes:

  • 5 secs for the data transmission - UI is ok
  • 15 secs for smth (thought it was json parsing but read next) - UI is completely frozen

now, without going to isolates, just made the same call using the standard http.Client() to fetch the json as plain string

  • 5 secs for the data transmission - UI is ok
  • 157 ms for MyResponseObject.fromJson(...) !!! Note, I'm calling the same fromJson factory retrofit does.

So, despite the JSON payload is huge, json is being parsed and deserialized very fast. I might even skip doing this in isolate (as this is a web app and no active UI interactions like scrolling, etc are made during this call)

Even if I go back to retrofit and adjust method to return plain string -

from Future<MyResponseObject> fetchData() to Future<String> fetchData() 

I see the same 15 secs freeze.

@trevorwang any idea what is causing this?

P.S.

retrofit: ^1.3.4+1
Flutter Channel stable, 2.0.6

@NicolaVerbeeck
Copy link
Collaborator

@livotov do you have a minimal self-contained example we can use to identify the root cause?

@livotov
Copy link

livotov commented May 6, 2021

@NicolaVerbeeck I think I can create one - this endpoint is public and contains no sensitive data.

@giaur500
Copy link

giaur500 commented May 6, 2021

Sorry, but I think you should create separate issue related to your problem. What retrofit is doing - you can look into auto generated code and you will know what is it.

Regarding to isolates support, is there any new idea?

@livotov
Copy link

livotov commented May 6, 2021

@giaur500 isolates support is in development for 2.0 as far as I can see.
#164

@livotov
Copy link

livotov commented May 6, 2021

Good idea for a separate ticket btw. @NicolaVerbeeck I'll create one and attach there a minimal sample app.

@livotov
Copy link

livotov commented May 6, 2021

@NicolaVerbeeck this appears to be a DIO issue - cfug/dio#961
so, not sure if we need a separate issue, however, as this makes retrofit unusable for requests, producing medium to large responses, this probably worth mentioning in the docs here.

@kunkaamd
Copy link

have any idea for this problem? my app is so slow when calling multiple APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.