Skip to content

Commit

Permalink
[@unimodules/react-native-adapter] Return Promise instead of awaiting…
Browse files Browse the repository at this point in the history
… it (#6882)

# Why

While performance testing #6881 I bumped into a strange performance difference between calling

```tsx
import * as Cellular from 'expo-cellular';

await Cellular.getCellularGenerationAsync();
```
and
```tsx
import { NativeModulesProxy } from '@unimodules/core';

await NativeModulesProxy.ExpoCellular.getCellularGenerationAsync();
```

where the latter was noticeably quicker (10%, 4188 ms vs 3764 ms).

After sharing my results with the team, @wkozyra95 has come up with an explanation — calling `return await promise` requires an extra event loop round comparing with `return promise`.

In situations where we don't wrap `return await` with a try-catch it should be safe to replace `return await` with `return`.

# How

- replaced `return await` with `return`,
- replaced `throw new Error` with `Promise.reject(new Error` not to throw errors outside of a Promise,
- removed `async` annotation from the function.

# Test Plan

Calling `await Application.getInstallReferrerAsync()` in a for loop 5000 times in `bare-expo` project resulted took:
- 78.5 s, **68.6 s**, 70.8 s, 70.7 s — **mean 72.1 s**

whereas on `master` the same benchmark took
- 75.5 s, **75.0 s**, 76.4 s, 75.6 s — **mean 75.6 s**

On average this PR should make calls to native about 4.5% faster.

Once this PR is accepted I will go through all other places where we `return await` and fix this issue there.
  • Loading branch information
sjchmiela authored and EvanBacon committed Feb 13, 2020
1 parent 14b5e10 commit fd4b8db
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 12 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,18 @@ if (NativeProxy) {
Object.keys(NativeProxy[exportedMethodsKey]).forEach(moduleName => {
NativeModulesProxy[moduleName] = NativeProxy[modulesConstantsKey][moduleName] || {};
NativeProxy[exportedMethodsKey][moduleName].forEach(methodInfo => {
NativeModulesProxy[moduleName][methodInfo.name] = async (
...args: unknown[]
): Promise<any> => {
NativeModulesProxy[moduleName][methodInfo.name] = (...args: unknown[]): Promise<any> => {
const { key, argumentsCount } = methodInfo;
if (argumentsCount !== args.length) {
throw new Error(
`Native method ${moduleName}.${methodInfo.name} expects ${argumentsCount} ${
argumentsCount === 1 ? 'argument' : 'arguments'
} but received ${args.length}`
return Promise.reject(
new Error(
`Native method ${moduleName}.${methodInfo.name} expects ${argumentsCount} ${
argumentsCount === 1 ? 'argument' : 'arguments'
} but received ${args.length}`
)
);
}
return await NativeProxy.callMethod(moduleName, key, args);
return NativeProxy.callMethod(moduleName, key, args);
};
});

Expand Down

0 comments on commit fd4b8db

Please sign in to comment.