Skip to content
This repository has been archived by the owner on May 3, 2023. It is now read-only.

Parallel Jetifier #24

Merged
merged 4 commits into from
Jun 30, 2019
Merged

Parallel Jetifier #24

merged 4 commits into from
Jun 30, 2019

Conversation

m4tt72
Copy link
Collaborator

@m4tt72 m4tt72 commented Jun 29, 2019

Jetify files in parallel using node child_process.
Removing the console.log from the loop makes the jetifier faster

@m4tt72 m4tt72 requested a review from mikehardy June 29, 2019 13:44
src/worker.js Outdated Show resolved Hide resolved
@mikehardy
Copy link
Owner

This seems cool but CI is choking on the ‘node’ version (that is ‘current’, which is v12.5 I think right now) doing the JS bundle. That seems like it shouldn’t have anything to do with this(!?), so I’ll investigate that

I will also see if I can add some sort of performance test (likely just wrapping the jetify call in time) so we can see what sort of boost this offers. I do like performance boosts but want to make sure the gain matches complexity in general - stability and longevity (which is related to a module’s ability to attract collaborators that can contribute easily) are goals of mine. I think this PR still looks reasonable from that perspective just want to make sure it really is achieving the gain that going parallel should achieve

@m4tt72
Copy link
Collaborator Author

m4tt72 commented Jun 29, 2019

Yes a performance test would be helpful!
On my machine, I gained +3 seconds when running the jetifier in parallel. In our project it went from 4 seconds to 800ms! not a huge performance boost but I like to squeeze every ms out of it...

@mikehardy
Copy link
Owner

3000 to 800ms is at minimum a 4x speedup, that’s motivating! I’m this close to smacking the approve button I just want to get CI working :-). Ah, CI problems.

@m4tt72
Copy link
Collaborator Author

m4tt72 commented Jun 29, 2019

@mikehardy the build fails probably because in the install script you are using npm instead of yarn. Try switching to yarn and see if that fixes the issue

@mikehardy
Copy link
Owner

Nah, relying on a package-manager difference to make things work is voodoo I think. npm and yarn are both capable (i use both, depending on the project, but npm in my biggest one - it’s fine).

it’s failing to bundle in RN0.60, not failing to jetify, something else is going on. I haven’t investigated yet, it’s still on the list as I work through items today. It will probably be simple, and I need to update rn-androidx-demo to RN0.60-RC3 anyway

Copy link
Owner

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

ok, I can reproduce a couple things locally, neither of which seem to be a problem with this PR. RN0.60 + ./gradlew :app:bundleReleaseJsAndAssets is taking a really really long time right now on linux (but not macOS it seems), and I can reproduce the missing dependency to node-pre-gyp on macOS + node v12. So this PR isn't blocked but I need to fix those things separately, and I'd like the messaging changes I proposed below? What do you think?

index.js Outdated Show resolved Hide resolved
src/worker.js Outdated Show resolved Hide resolved
src/worker.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
Copy link
Owner

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Just about have CI all whipped into shape and re-checked this, all looks good. I’ll merge my CI tweaks then re-run this and all should be green (CI, reviews) and fast (this PR). Awesome man

@mikehardy
Copy link
Owner

Just for reference for later - the current rn-androidx-demo test suite is running at 3.4-4s forward and (because the build folder is huge) about 20s in reverse

Once I commit the CI items and merge/re-run this we'll have the competitive timings from the update. I predict ~1s forward and 5s reverse.

@mikehardy mikehardy mentioned this pull request Jun 30, 2019
@mikehardy mikehardy merged commit 0a16472 into master Jun 30, 2019
@mikehardy
Copy link
Owner

Well, timing was right on for scaling by CPUs with 2 CPUs, so in CI it was 2x faster - ~2s and ~10s. Nice to see it scales like that actually. This is a great addition, I can’t think of a single reason at this point why someone would object to relying on this to ease the AndroidX pain. Thanks @m4tt72

@m4tt72 m4tt72 deleted the parallel-jetifier branch June 30, 2019 08:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants