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

Utilise compute() #158

Closed
nodinosaur opened this issue Aug 7, 2020 · 21 comments
Closed

Utilise compute() #158

nodinosaur opened this issue Aug 7, 2020 · 21 comments
Assignees
Labels
documentation Documentation related enhancement New feature or request

Comments

@nodinosaur
Copy link

With Chopper is there a way that we can utilise Isolates / compute() for loading and parsing large documents?

https://flutter.dev/docs/cookbook/networking/background-parsing

https://api.flutter.dev/flutter/dart-isolate/Isolate-class.html

I have seen that some Devs are moving to Dio as it can already do this 👉 README.md, but I am quite invested in Chopper and don't really want to rework my network layer.

Is it possible, or could such a feature be added?

Thanks!

@nodinosaur
Copy link
Author

@stewemetal stewemetal self-assigned this Aug 23, 2020
@stewemetal stewemetal added enhancement New feature or request documentation Documentation related labels Aug 23, 2020
@stewemetal
Copy link
Collaborator

We'll look into implementing this feature ASAP.

@JEuler @lejard-h we should look into how Dio does it (thanks @nodinosaur for the readme link!) and implement our own solution.

@lejard-h
Copy link
Owner

It should be possible via Converter but never tested

class FlutterConverter implements Converter {
    Future<Response> decodeJson<BodyType, InnerType>(Response response) {
      var contentType = response.headers[contentTypeKey];
      var body = response.body;
      if (contentType != null && contentType.contains(jsonHeaders)) {
        body = utf8.decode(response.bodyBytes);
      }

      body = _tryDecodeJson(body);
      if (isTypeOf<BodyType, Iterable<InnerType>>()) {
        body = body.cast<InnerType>();
      } else if (isTypeOf<BodyType, Map<String, InnerType>>()) {
        body = body.cast<String, InnerType>();
      }

      return response.copyWith<BodyType>(body: body);
    }
    
    ///
    /// Compute HERE
    ///
    Future<dynamic> _tryDecodeJson(String data) {
      try {
        return compute(json.decode, data);
      } catch (e) {
        chopperLogger.warning(e);
        return data;
      }
    }

    Future<Response<BodyType>> convertResponse<BodyType, InnerType>(
      Response response,
    ) async {
      return decodeJson<BodyType, InnerType>(response);
    }
}

I partially copied JsonConverter, it would be easier to implements if we had FutureOr on every JsonConverter functions and if _tryDecodeJson was public

class FlutterConverter extends JsonConverter {
  Future<dynamic> tryDecodeJson(String data) {
    return compute(tryDecodeJson, data);
  }
}

@DevNico
Copy link

DevNico commented Mar 29, 2021

Did anything ever get merged / what is the way to go if usage of compute is desired?

@nodinosaur
Copy link
Author

I am still hoping that there will be some official documentation with a recommended example on this, I could not get the above example to work 🤔

@JanKn
Copy link

JanKn commented Jan 6, 2022

I got it working, I can see isolates being spawned and the data is loaded in our app.
Let me know if that works for you too @nodinosaur .

Two steps:

  1. Creating ComputeConverter:
class ComputeConverter extends Converter {
  ComputeConverter(this.converter);

  final JsonSerializableConverter converter;

  @override
  FutureOr<Request> convertRequest(Request request) {
    return converter.convertRequest(request);
  }

  @override
  FutureOr<Response<BodyType>> convertResponse<BodyType, InnerType>(Response response) {
    return compute((Response response) => converter.convertResponse<BodyType, InnerType>(response), response);
  }
}
  1. And then wrapping the JsonSerializableConverter with ComputeConverter in the Chopper Client setup:
final client = ChopperClient(
      baseUrl: baseUrl,
      converter: ComputeConverter(const JsonSerializableConverter(
        {
          XData: XData.fromJson,
          ...
        },
      )),
      interceptors: [
        HttpRequestLoggingInterceptor(),
      ],
    );

@vaetas
Copy link

vaetas commented Mar 2, 2022

Hey everybody,
I want to note one thing: compute starts a new Isolate for each call. Starting new Isolate may take between 100-200 milliseconds. Each Isolate also copies some memory therefore having many Isolates might increase memory load of your app.

Example from @JanKn works well if you have a demanding requests or response once in a while. If your responses are small, using compute might actually increase the real time required to process it (at least it will not block the UI isolate). And if you have many concurrent requests then compute will create many Isolates at the same time (therefore increasing memory footprint).

The best solution I found seems to be spawning one Isolate at the app startup and then using it only for converting requests & responses. This can be done using isolate package (that is for unknown reason discontinued, but I don't think it significantly matters).

You spawn the IsolateRunner in your main and then use it inside the converter. Spawning takes about 200ms (debug mode, iOS simulator).

late final IsolateRunner isolateRunner;

Future<void> main() async {
  isolateRunner = await IsolateRunner.spawn();
  runApp(const ExampleApp());
}
import 'package:isolate/isolate_runner.dart';

class _IsolateConverter extends Converter {
  _IsolateConverter(
    this.converter, {
    this.isolateRunner,
  });

  final JsonSerializableConverter converter;
  final IsolateRunner? isolateRunner;

  bool get runIsolate => isolateRunner != null;

  @override
  FutureOr<Request> convertRequest(Request request) {
    if (runIsolate) {
      return isolateRunner!.run(converter.convertRequest, request);
    } else {
      return converter.convertRequest(request);
    }
  }

  @override
  FutureOr<Response<BodyType>> convertResponse<BodyType, InnerType>(
    Response response,
  ) {
    if (runIsolate) {
      return isolateRunner!.run(
        (argument) {
          return converter.convertResponse<BodyType, InnerType>(
            argument as Response<dynamic>,
          );
        },
        response,
      );
    } else {
      return converter.convertResponse<BodyType, InnerType>(response);
    }
  }
}

My code example falls back to main Isolate when you do not provide custom IsolateRunner. I want to run some real-life tests with and without IsolateRunner in the future.

This IsolateRunner can probably be used for different computations so it is not idle when no JSON parsing is used. Again, this would probably depend on your use case.

There are also few packages for isolate (thread) pooling which allow you to spawn multiple Isolates at startup (say 2 or 4) and then split the work load between them. This seems overkill for my JSON parsing.

@thaihuynhxyz
Copy link

I think we should apply something like ThreadPool. Spam spawn Isolates can lead to Out of memory crashes. Take a look at squadron.

@techouse
Copy link
Collaborator

techouse commented Jul 11, 2022

Any news on this one?

As @lejard-h pointed out above, it would be very helpful if all the JsonConverter methods had a FutureOr return type and the _tryDecodeJson were made public.

EDIT: I've started adding these types to a fork here https://github.com/techouse/chopper/tree/futureor-json-converter and will play around with compute on them tomorrow. This is however quite the breaking change as it will force people to change the return types from Response to FutureOr<Response> and migrate their code to async/await.

EDIT 2: My preliminary tests using the above fork seem to work fine with and without compute the only thing the user has to do is edit their JsonConverter's convertResponse and convertError to return FutureOr<Response<ResultType>> and FutureOr<Response> respectively.

I'll make a draft PR.

@JEuler
Copy link
Collaborator

JEuler commented Jul 14, 2022

Thank you so much for your contribution. I will have a look soon!

@techouse
Copy link
Collaborator

Thank you so much for your contribution. I will have a look soon!

Sure, no problem 😊 All the breaking changes are limited to classes extending JsonConverter so it's not that bad, unless one has many JsonConverters. 🙈

@vaetas
Copy link

vaetas commented Jul 14, 2022

@techouse have you tried calling compute method multiple times at the same time? I am afraid that many concurrent compute calls can negatively impact performance in different way (esp. memory consumption).

Apps can easily run many API calls on startup. I would be careful with feature that can have unexpected results.

I never really managed to do the benchmark myself. It would be good to start discussion about this. Also, the implementation might have changed since then.

@techouse
Copy link
Collaborator

@vaetas I did. But only something like 5-10. It worked just fine on a real iPhone or Android. The simulator however is a different story.

Nonetheless, this PR just wraps the Response in a FutureOr. It doesn't introduce any compute by itself.

@vaetas
Copy link

vaetas commented Jul 14, 2022

@techouse I understand that your PR allows async converters. This will be useful for any Isolate implementation. Thanks!

I just wanted to get some discussion going for the next steps. I will try to benchmark compute method in real large-ish Flutter app to see real memory usage. We managed to fix crashes on low-end Android devices using dedicated Isolate for JSON decoding. My only worry is that spamming compute might cause memory problems on these Android devices.

@techouse
Copy link
Collaborator

techouse commented Jul 15, 2022

@vaetas Indeed, compute will need furhter benchmarking, however, at we need to get the ball rolling and #345 should help kick things off. 😊

@siddhesh-boppo
Copy link

Hello, @vaetas I tried to play around json converter, but didn't find solution. I have to do concurrent API call and parse json on isolate and return to main thread with parsed data.

import 'dart:convert';

import 'package:built_value/serializer.dart';
import 'package:built_value/standard_json_plugin.dart';
import 'package:chopper/chopper.dart';
import 'package:built_collection/built_collection.dart';
import 'package:isolate/isolate_runner.dart';

import '../../main.dart';
import 'serializers.dart'; // for isolateRunner

class BuiltValueConverter implements Converter, ErrorConverter {
  final Type? errorType;

  BuiltValueConverter({this.errorType});

  final jsonSerializers =
      (serializers.toBuilder()..addPlugin(StandardJsonPlugin())).build();

  T? _deserializer<T>(dynamic value) => jsonSerializers.deserializeWith(
        jsonSerializers.serializerForType(T) as Serializer<T>,
        value,
      );

  bool get runIsolate => isolateRunner != null;

  @override
  FutureOr<Request> convertRequest(Request request) {
    final req = applyHeader(
      request,
      contentTypeKey,
      jsonHeaders,
      override: false,
    );

    return encodeJson(req);
    // return request;
  }

  Request encodeJson(Request request) {
    var contentType = request.headers[contentTypeKey];
    if (contentType != null && contentType.contains(jsonHeaders)) {
      return request.copyWith(body: json.encode(request.body));
    }
    return request;
  }

  @override
  FutureOr<Response<ResultType>> convertResponse<ResultType, InnerType>(
    Response response,
  ) {
    // final jsonResponse = super.convertResponse(response);
    print(response);

    if (runIsolate) {
      final body = _decode<InnerType>(response.body);

      return isolateRunner.run((args) {
        return response.copyWith<ResultType>(body: body);
      }, response);
    } else {
      final body = _decode<InnerType>(response.body);
      return response.copyWith<ResultType>(body: body);
    }
    // return jsonResponse.copyWith<ResultType>(body: body);
  }

  dynamic _decode<T>(entity) {
    // debugPrint(entity); // debug print causes problem with chopper
    print(entity);

    /// handle case when we want to access to Map<String, dynamic> directly
    /// getResource or getMapResource
    /// Avoid dynamic or unconverted value, this could lead to several issues
    if (entity is T) return entity;

    try {
      json.decode(entity);
      if (entity is List) return _deserializeListOf<T>(entity);
      return _deserializer<T>(entity);
    } catch (e) {
      print(e);
      return null;
    }
  }

  Response decodeJson<BodyType, InnerType>(Response response) {
    final supportedContentTypes = [jsonHeaders, jsonApiHeaders];

    final contentType = response.headers[contentTypeKey];
    var body = response.body;

    if (supportedContentTypes.contains(contentType)) {
      // If we're decoding JSON, there's some ambiguity in https://tools.ietf.org/html/rfc2616
      // about what encoding should be used if the content-type doesn't contain a 'charset'
      // parameter. See https://github.com/dart-lang/http/issues/186. In a nutshell, without
      // an explicit charset, the Dart http library will fall back to using ISO-8859-1, however,
      // https://tools.ietf.org/html/rfc8259 says that JSON must be encoded using UTF-8. So,
      // we're going to explicitly decode using UTF-8... if we don't do this, then we can easily
      // end up with our JSON string containing incorrectly decoded characters.
      body = utf8.decode(response.bodyBytes);
    }

    body = _tryDecodeJson(body);
    if (isTypeOf<BodyType, Iterable<InnerType>>()) {
      body = body.cast<InnerType>();
    } else if (isTypeOf<BodyType, Map<String, InnerType>>()) {
      body = body.cast<String, InnerType>();
    }

    return response.copyWith<BodyType>(body: body);
  }

  dynamic _tryDecodeJson(String data) {
    try {
      return json.decode(data);
    } catch (e) {
      chopperLogger.warning(e);
      return data;
    }
  }

  BuiltList<T> _deserializeListOf<T>(Iterable value) => BuiltList(
        value.map((value) => _deserializer<T>(value)).toList(growable: true),
      );

  @override
  FutureOr<Response<BodyType>> convertError<BodyType, InnerType>(
      Response response) {
    // final response = decodeJson<BodyType, InnerType>(response1) as Response<BodyType>;
    // final response =
    //     JsonConverter().convertResponse<BodyType, InnerType>(response1);
    print('my response ${response.body}');
    // final jsonResponse = super.convertResponse(response);

    final body = _decode(response.body);

    if (runIsolate) {
      return isolateRunner.run((args) {
        return response.copyWith<BodyType>(body: body);
      }, response);
    } else {
      return response.copyWith<BodyType>(body: body);
    }
    // return response.copyWith(body: body);
  }
}

@techouse
Copy link
Collaborator

techouse commented Sep 7, 2022

@vaetas #345 is now in origin/develop and @JEuler will soon make a major release, so you can start hammering your isolate ideas into the repo 🥳

@JEuler
Copy link
Collaborator

JEuler commented Sep 8, 2022

#352 - now we need to merge this and after that, I will create PR for the master branch which will upgrade the pub.dev version. :)

@JEuler
Copy link
Collaborator

JEuler commented Sep 13, 2022

It seems that 5.0.0 version is on pub.dev :)

@techouse
Copy link
Collaborator

techouse commented Sep 13, 2022

I think we should apply something like ThreadPool. Spam spawn Isolates can lead to Out of memory crashes. Take a look at squadron.

I must say squadron is amazing and the Flutter example is a good starting point.

I've just implemented it using a worker pool in a live app together with Chopper v5.0.0 and it's just amazing.

Check out the example here on how to implement it in about 30min.

@techouse
Copy link
Collaborator

I wrote an example using Squadron here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

10 participants