perf(Android): getConstants() in thread to parallelize load, speed up for most #680

mcuelenaere · 2019-06-03T12:44:17Z

Description

RNDeviceModule.getConstants() can take a lot of time (~300ms on a Galaxy S7), so try to run it as soon as possible in the background so that by the time it gets called, the constants have already been calculated and we can just return the map.

Checklist

I have tested this on a device/simulator for each compatible OS
I added the documentation in README.md
I mentioned this change in CHANGELOG.md

noomorph · 2019-06-03T13:12:37Z

What are your measurements (before vs after) and how did you measure time-to-load?

mcuelenaere · 2019-06-03T13:35:27Z

This was measured by adding a ReactMarker listener and printing out durations of each step, where we focus here only on the GET_CONSTANTS part of RNDeviceInfo.

Below are 3 measurements from before and after the patch, each starting from a cold environment (app was killed). As you can see, the GET_CONSTANTS duration gets reduced to almost nothing.

BEFORE:

   +325.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
   +229.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +6.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

   +288.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
   +265.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +3.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

   +286.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
   +262.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +4.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

AFTER:

   +276.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
     +0.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +1.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

   +382.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
     +0.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +1.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

   +280.000000 ms --                    GET_CONSTANTS [START]: RNDeviceInfo
     +0.000000 ms --                CONVERT_CONSTANTS [START]: RNDeviceInfo
     +1.000000 ms --                  CONVERT_CONSTANTS [END]: RNDeviceInfo
     +0.000000 ms --                      GET_CONSTANTS [END]: RNDeviceInfo

Your mileage may vary, since this depends on how much time is in between the RNDeviceModule constructor being called and the getConstants() method being invoked (+ scheduling and thread synchronization overhead).

tl;dr: if you have a decent amount of RN modules being loaded in your app (and your device has multiple cores), this should have a positive effect.

mikehardy

Nice workaround! even with a move to a Promise style as desired in the next major rev (was planned for v2 but we had a breaking change prior, so v3) this might have value as a "constants warmer". I can't imagine it is common, but it is possible that converting this getConstants() time from serial to parallel may negatively impact people. I don't mind making this the default behavior, but how hard would it be to make two constructors, one that takes no arguments and is default to "warm constants" and one that allows a boolean and let's people defer the cost to first getConstants() invocation like existing behavior?

If it is too difficult I'd scrap the idea, but I think it might be easy assuming getConstants() will always fill lazily if the AsyncTask hasn't been called yet

That way for the few that may have taken great pains to avoid this cost in some other way, we won't be blowing up whatever work they've done

What do you think?

mikehardy · 2019-06-03T15:01:04Z

just marking it as "needs-changes" because that's a tag I review periodically even when PRs go to sleep

mcuelenaere · 2019-06-03T15:09:47Z

@mikehardy the behaviour is optional now and disabled by default

mikehardy · 2019-06-03T15:16:17Z

How did I miss that? Let me re-read, because that sounds perfect...

mikehardy · 2019-06-03T15:17:19Z

Oh you just did it haha, thought I was losing my mind. This does sound perfect now, I will re-read and probably merge

Thanks for the contribution!

mikehardy · 2019-06-03T15:49:51Z

android/src/main/java/com/learnium/RNDeviceInfo/RNDeviceModule.java

+    this(reactContext, false);
+  }
+
+  public RNDeviceModule(ReactApplicationContext reactContext, boolean loadConstantsAsynchronously) {


This makes it optional, but now for this method to be invoke with async load, does it need some corresponding hook in RNDeviceInfo.java? Like RNDeviceInfo needs two constructors with a similar flag, it stores the state, then when RNDeviceInfo.createNativeModules is called it sends the flag through?

Otherwise I'm not sure how the new async style is invoked since the default (and only currently available) style from linking is to just new RNDeviceInfo and there are no other control flow entrypoints to toggle the behavior https://github.com/mcuelenaere/react-native-device-info/blob/55dc03dc46/example/android/app/src/main/java/com/example/MainApplication.java#L27

Sorry to complicate things for what is probably a non-issue statistically but I still think it's quickly achievable :-)

Oh, you're right. It's because locally I'm using Turbo Modules and thus creating the RNDeviceModule myself. I'll adapt RNDeviceInfo to also take a constructor argument.

mikehardy · 2019-06-03T15:51:14Z

I pulled this locally and have it ready for testing - I think it needs one more tweak after I requested it be optional then it's probably good to go

…time RNDeviceModule.getConstants() can take a lot of time (~300ms on a Galaxy S7), so try to run it as soon as possible in the background so that by the time it gets called, the constants have already been calculated and we can just return the map.

mcuelenaere · 2019-06-03T15:56:16Z

@mikehardy loadConstantsAsynchronously is now propagated from RNDeviceInfo to RNDeviceModule

mikehardy · 2019-06-03T15:56:40Z

I'll repull and retest - super-speedy thanks!

mikehardy · 2019-06-03T16:10:36Z

https://github.com/react-native-community/react-native-device-info/releases/tag/v2.1.0 🚀

ItsNoHax · 2019-06-18T12:38:38Z

@mikehardy @mcuelenaere First of all thanks a lot to both of you for the fix. I would just like to point out that the change log and the README should reflect these changes. I'm still unclear on whether I should use true or false in the constructor.

mikehardy · 2019-06-18T12:49:17Z

@ItsNoHax the release notes do mention it? https://github.com/react-native-community/react-native-device-info/blob/master/CHANGELOG.md#210 (they say use true)

I suppose the README for install could be updated, it might help to explain that the constructor even has an optional boolean argument, and what it means. Happy to take a PR for that - you can edit it directly using the GitHub UI and propose some words that you think would help people?

ItsNoHax · 2019-06-18T13:23:45Z

@mikehardy Done, let me know what you think: #697

I left it under the main installation header as I was afraid people might miss it when linking automatically.

elyalvarado · 2019-07-12T23:58:02Z

Hi guys, how will this work with RN>=0.60 auto loader?

jslok · 2019-07-29T12:52:04Z

Is there a way to make this work with autolinking or do we need to manually link to use this feature? Thank you

mikehardy · 2019-07-29T13:10:44Z

@jslok to be honest I am not sure - but I am curious myself as I am just migrating my work project to RN60 now.

My current understanding is that to initialize a module with a non-default constructor (specifically: one that takes an argument) in the auto-linking world, you have to use a react-native-config.js to disable autolinking, then manually link it on the platform you disabled.

@thymikee is that correct? auto-linking only works with default constructors?

I initially merged this patch as a new option and did not set the deferred method to default but assuming my understanding of auto-linking is correct and it will only work with a default constructor, I'd accept a patch that reversed which style was default now that it has been released a while and shown to be working.

thymikee · 2019-07-29T13:29:36Z

@mikehardy disabling linking in react-native.config.js affects both autolinking and link.

Passing a custom constructor is possible through android.packageInstance:

module.exports = {
  dependencies: {
    'react-native-device-info': {
      platforms: {
        android: {
          packageInstance: 'new RNDeviceInfo(true)'
        }
      }
    }
  }
};

mikehardy · 2019-07-29T13:38:21Z

@thymikee I really appreciate both your responsiveness, and your spot-on response, thank you! I promise I'll be good at auto-linking soon :-)

Okay then, @jslok I would happily take either a PR that made the parallel version default + a quick doc snippet on how to disable it (with the snippet above I think but reversed to false) or a doc enhancement that had this snippet showing how to enable it.

I think it's ready for default though, I believe more will benefit from it's being enabled than will be harmed so I would prefer just enabling by default

Dror-Bar · 2019-08-04T14:38:39Z

I don't have a file named react-native.config.js. It is unclear from the readme whether I should create it myself and where.

I'm using isTablet() method to calculate UI elemens in the first screen. So I need to load it synchronically.

mikehardy · 2019-08-04T14:44:02Z

@Dror-Bar if you wanted to propose a quick readme PR that made it clear to you that would help. Specifically addressed at the confusion: the file does not exist in the repository, but if you do not want the parallel initialization, you will need to create the file, and it should contain the stanza prescribed in the react-native-config.js in the readme

If you are okay with the new default / parallel initialization is okay for your project, you do not need to do anything

My decision to merge this and alter the default was based on the assumption it was reasonably tested (so would not break anyone) and that in general it was a performance increase for everyone or at least did not harm things. If this is not true for your project I would love to know why in order to see if my assumption was correct - and I'd apologize for regressing your project also, that definitely wasn't intended

mcuelenaere mentioned this pull request Jun 3, 2019

require('react-native-device-info') import is too long and synchronous #517

Closed

mikehardy requested changes Jun 3, 2019

View reviewed changes

mikehardy self-assigned this Jun 3, 2019

mikehardy added android enhancement ready-for-review reviewed-needs-changes labels Jun 3, 2019

mcuelenaere force-pushed the feature/async-getconstants branch from 2f1d4c4 to 55dc03d Compare June 3, 2019 15:09

mikehardy reviewed Jun 3, 2019

View reviewed changes

mcuelenaere force-pushed the feature/async-getconstants branch from 55dc03d to b0f7500 Compare June 3, 2019 15:55

mcuelenaere requested review from GantMan and machour as code owners June 3, 2019 15:55

mikehardy approved these changes Jun 3, 2019

View reviewed changes

mikehardy changed the title ~~Android: run getConstants() in a different thread to speedup loading time~~ perf(Android): getConstants() in thread to parallelize load, speed up for most Jun 3, 2019

mikehardy merged commit 2f837b9 into react-native-device-info:master Jun 3, 2019

ItsNoHax mentioned this pull request Jun 18, 2019

docs(README): Clarify the new changes regarding loading constants async #697

Merged

react-native-device-info deleted a comment from zhao812 Jul 12, 2019

jslok mentioned this pull request Jul 29, 2019

Make async loading default #741

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(Android): getConstants() in thread to parallelize load, speed up for most #680

perf(Android): getConstants() in thread to parallelize load, speed up for most #680

mcuelenaere commented Jun 3, 2019 •

edited

Loading

noomorph commented Jun 3, 2019 •

edited

Loading

mcuelenaere commented Jun 3, 2019 •

edited

Loading

mikehardy left a comment

mikehardy commented Jun 3, 2019

mcuelenaere commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy Jun 3, 2019

mikehardy Jun 3, 2019

mcuelenaere Jun 3, 2019 •

edited

Loading

mikehardy commented Jun 3, 2019

mcuelenaere commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy commented Jun 3, 2019

ItsNoHax commented Jun 18, 2019

mikehardy commented Jun 18, 2019

ItsNoHax commented Jun 18, 2019 •

edited

Loading

elyalvarado commented Jul 12, 2019

jslok commented Jul 29, 2019

mikehardy commented Jul 29, 2019

thymikee commented Jul 29, 2019

mikehardy commented Jul 29, 2019

Dror-Bar commented Aug 4, 2019 •

edited

Loading

mikehardy commented Aug 4, 2019

perf(Android): getConstants() in thread to parallelize load, speed up for most #680

perf(Android): getConstants() in thread to parallelize load, speed up for most #680

Conversation

mcuelenaere commented Jun 3, 2019 • edited Loading

Description

Checklist

noomorph commented Jun 3, 2019 • edited Loading

mcuelenaere commented Jun 3, 2019 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Jun 3, 2019

mcuelenaere commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy Jun 3, 2019

Choose a reason for hiding this comment

mikehardy Jun 3, 2019

Choose a reason for hiding this comment

mcuelenaere Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

mikehardy commented Jun 3, 2019

mcuelenaere commented Jun 3, 2019

mikehardy commented Jun 3, 2019

mikehardy commented Jun 3, 2019

ItsNoHax commented Jun 18, 2019

mikehardy commented Jun 18, 2019

ItsNoHax commented Jun 18, 2019 • edited Loading

elyalvarado commented Jul 12, 2019

jslok commented Jul 29, 2019

mikehardy commented Jul 29, 2019

thymikee commented Jul 29, 2019

mikehardy commented Jul 29, 2019

Dror-Bar commented Aug 4, 2019 • edited Loading

mikehardy commented Aug 4, 2019

mcuelenaere commented Jun 3, 2019 •

edited

Loading

noomorph commented Jun 3, 2019 •

edited

Loading

mcuelenaere commented Jun 3, 2019 •

edited

Loading

mcuelenaere Jun 3, 2019 •

edited

Loading

ItsNoHax commented Jun 18, 2019 •

edited

Loading

Dror-Bar commented Aug 4, 2019 •

edited

Loading