Skip to content

perf(autolinking)!: prioritise files with *Package.java|*Package.kt in getPackageClassName #2384

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.

Sign up for GitHub

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

Merged
thymikee merged 3 commits into from
Jun 7, 2024
Merged

perf(autolinking)!: prioritise files with *Package.java|*Package.kt in getPackageClassName #2384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12bad6e
perf(autolinking)!: prioritise files with `*Package.java|*Package.kt`…
szymonrybczak May 8, 2024
6806649
fix: get all files when getting files inside project
szymonrybczak May 10, 2024
73e203c
test: add tests
szymonrybczak Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/cli-platform-android/src/config/__fixtures__/android.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ export const valid = generateValidFileStructureForLib('ReactPackage.java');

export const validKotlin = generateValidFileStructureForLib('ReactPackage.kt');

export const validWithDifferentFileName =
generateValidFileStructureForLib('React.java');

export const validKotlinWithDifferentFileName =
generateValidFileStructureForLib('React.kt');

export const validApp = generateValidFileStructureForApp();

export const userConfigManifest = {
Expand Down
34 changes: 34 additions & 0 deletions packages/cli-platform-android/src/config/__fixtures__/files/React.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

package com.some.example;

import com.facebook.react.ReactPackage;
import com.facebook.react.bridge.NativeModule;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.uimanager.ViewManager;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

public class SomeExampleJavaPackage implements ReactPackage {

@Override
public List<NativeModule> createNativeModules(
ReactApplicationContext reactContext) {
List<NativeModule> modules = new ArrayList<>();
modules.add(new SomeExampleModule(reactContext));
return modules;
}

@Override
public List<ViewManager> createViewManagers(ReactApplicationContext reactContext) {
return Collections.emptyList();
}
}
26 changes: 26 additions & 0 deletions packages/cli-platform-android/src/config/__fixtures__/files/React.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

package com.some.example;

import android.view.View
import com.facebook.react.ReactPackage
import com.facebook.react.bridge.NativeModule
import com.facebook.react.bridge.ReactApplicationContext
import com.facebook.react.uimanager.ReactShadowNode
import com.facebook.react.uimanager.ViewManager
import java.util.*

class SomeExampleKotlinPackage : ReactPackage {

override fun createNativeModules(reactContext: ReactApplicationContext): MutableList<NativeModule>
= mutableListOf(MaterialPaletteModule(reactContext))

override fun createViewManagers(reactContext: ReactApplicationContext?):
MutableList<ViewManager<View, ReactShadowNode>> = Collections.emptyList()

}
18 changes: 18 additions & 0 deletions packages/cli-platform-android/src/config/__tests__/findPackageClassName.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,15 @@ const fs = require('fs');
flatJava: {
android: mocks.valid,
},
flatJavaDifferentName: {
android: mocks.validWithDifferentFileName,
},
flatKotlin: {
android: mocks.validKotlin,
},
flatKotlinDifferentName: {
android: mocks.validKotlinWithDifferentFileName,
},
},
platform,
);
Expand All @@ -42,12 +48,24 @@ const fs = require('fs');
);
});

it('returns the name of the java class implementing ReactPackage with different file name', () => {
expect(findPackageClassName(`${root}flatJavaDifferentName`)).toBe(
'SomeExampleJavaPackage',
);
});

it('returns the name of the kotlin class implementing ReactPackage', () => {
expect(findPackageClassName(`${root}flatKotlin`)).toBe(
'SomeExampleKotlinPackage',
);
});

it('returns the name of the kotlin class implementing ReactPackage with different file name', () => {
expect(findPackageClassName(`${root}flatKotlinDifferentName`)).toBe(
'SomeExampleKotlinPackage',
);
});

it('returns `null` if there are no matches', () => {
expect(findPackageClassName(`${root}empty`)).toBeNull();
});
Expand Down
35 changes: 28 additions & 7 deletions packages/cli-platform-android/src/config/findPackageClassName.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,43 @@ import glob from 'fast-glob';
import path from 'path';
import {unixifyPaths} from '@react-native-community/cli-tools';

export function getMainActivityFiles(folder: string) {
return glob.sync('**/+(*.java|*.kt)', {cwd: unixifyPaths(folder)});
export function getMainActivityFiles(
folder: string,
includePackage: boolean = true,
) {
let patternArray = [];

if (includePackage) {
patternArray.push('*Package.java', '*Package.kt');
} else {
patternArray.push('*.java', '*.kt');
}

return glob.sync(`**/+(${patternArray.join('|')})`, {
cwd: unixifyPaths(folder),
});
}

export default function getPackageClassName(folder: string) {
const files = getMainActivityFiles(folder);
let files = getMainActivityFiles(folder);
let packages = getClassNameMatches(files, folder);

const packages = files
.map((filePath) => fs.readFileSync(path.join(folder, filePath), 'utf8'))
.map(matchClassName)
.filter((match) => match);
if (!packages.length) {
files = getMainActivityFiles(folder, false);
Copy link
Member

@satya164 satya164 May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe there's scope to exclude previously matched files from this list not to read same files twice

Copy link
Collaborator Author

@szymonrybczak szymonrybczak May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! good catch

Comment on lines +32 to +36
Copy link
Contributor

@tido64 tido64 Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit late to the game but I wonder if globbing for **/*.{java,kt} once and filtering in-memory instead of hitting the disk twice is faster for bigger projects.

Copy link
Member

@thymikee thymikee Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szymonrybczak keen to measure it and sending a followup if that helps with perf? :)

Copy link
Collaborator Author

@szymonrybczak szymonrybczak Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I gave it a shot but the improvement won't be big because after my checks the amount of libraries that needs to fallback are very small.

Now:

❯ hyperfine "npx react-native config --platform android"
Benchmark 1: npx react-native config --platform android
  Time (mean ± σ):      2.070 s ±  0.264 s    [User: 1.049 s, System: 0.607 s]
  Range (min … max):    1.916 s …  2.814 s    10 runs

With executing glob once and filtering in-memory:

❯ hyperfine "npx react-native config --platform android"
Benchmark 1: npx react-native config --platform android
  Time (mean ± σ):      2.096 s ±  0.190 s    [User: 1.078 s, System: 0.620 s]
  Range (min … max):    1.931 s …  2.572 s    10 runs

packages = getClassNameMatches(files, folder);
}

// @ts-ignore
return packages.length ? packages[0][1] : null;
}

function getClassNameMatches(files: string[], folder: string) {
return files
.map((filePath) => fs.readFileSync(path.join(folder, filePath), 'utf8'))
Copy link
Contributor

@tido64 tido64 May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if streaming is faster than reading the whole file every time: https://tangledeveloper.medium.com/how-to-read-file-line-by-line-efficiently-in-node-js-8125fe5bd185

Copy link
Member

@thymikee thymikee May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth to measure definitely, these declarations are usually somewhere at the top of the file so it hints some potential

Copy link
Contributor

@tido64 tido64 May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing glob.sync above with glob.stream might also be worth looking into, but making everything async might turn this into a very big PR 😛

.map(matchClassName)
.filter((match) => match);
}

export function matchClassName(file: string) {
const nativeModuleMatch = file.match(
/class\s+(\w+[^(\s]*)[\s\w():]*(\s+implements\s+|:)[\s\w():,]*[^{]*ReactPackage/,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-platform-android/src/config/isProjectUsingKotlin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {getMainActivityFiles} from './findPackageClassName';

export default function isProjectUsingKotlin(sourceDir: string): boolean {
const mainActivityFiles = getMainActivityFiles(sourceDir);
const mainActivityFiles = getMainActivityFiles(sourceDir, false);

return mainActivityFiles.some((file) => file.endsWith('.kt'));
}