-
Notifications
You must be signed in to change notification settings - Fork 883
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
perf(autolinking)!: prioritise files with *Package.java|*Package.kt
in getPackageClassName
#2384
Conversation
|
||
// @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')) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); | ||
if (!packages.length) { | ||
files = getMainActivityFiles(folder, false); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement 👍 All good on my end but wait for another CLI reviews before merging this (also because I believe this is not E2E tested)
e93df86
to
6806649
Compare
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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
Summary:
This Pull Request changes schema that looks for files to check to find class name, after my tests in various application the match results were included almost in every library in files with
Package
at the end. But our previous logic was going through every file in library with.kt
and.java
extension until it found file withimplements ReactPackage
, some libraries can have 100+ files and we were reading them until we matched relevant Regex schema 😅I also added a fallback which checks files with
*.kt
and*.java
extensions if it couldn't findimplements ReactPackage
in file with*Package
at the end, which ensures us that we don't introduce any breaking change. The output ofconfig
command stays the same.Performance improvement
I did a test in Expensify app, which is an open-source app with 50 libraries containing native code. Before my changes, we were reading 490 files to find the class name. Right now, we only check 79 (still there's a room for improvements).
Test Plan:
The
dependencies
field should be the same as before.Checklist