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

perf: Introduce parallelism for jetifier #5

Merged
merged 15 commits into from
Jun 20, 2019
Merged

perf: Introduce parallelism for jetifier #5

merged 15 commits into from
Jun 20, 2019

Conversation

cawfree
Copy link
Contributor

@cawfree cawfree commented Jun 19, 2019

Hey @mikehardy, added a couple of teaks that seem to work fine on my production application. I should probably note that it has a non-trivial number of dependencies, i.e. react-native-firebase, react-native-camera etc. It appears to compile and run successfully after processing unjetified dependencies. #3

@mikehardy
Copy link
Owner

I have unfortunately found a case it may fail on: react-native-sqlite-storage includes these (and more like them) in it's distribution

jetifying file node_modules/react-native-sqlite-storage/src/android/build/generated/source/buildConfig/debug/org/pgsqlite/BuildConfig.java
jetifying file node_modules/react-native-sqlite-storage/src/android/build/generated/source/r/debug/android/support/v4/R.java
jetifying file node_modules/react-native-sqlite-storage/src/android/build/generated/source/r/debug/android/support/v7/appcompat/R.java
jetifying file node_modules/react-native-sqlite-storage/src/android/build/generated/source/r/debug/android/support/v7/recyclerview/R.java

I'm not sure if they are totally needed or not? Might make a good test case though, and I have this test bed now: https://github.com/mikehardy/rn-androidx-demo - you can fork + clone that one and quickly alter the jetifier dependency to your git branch and try it on sqlite? May need to alter App.js to actually exercise it I'm not sure...

bin/jetify Outdated Show resolved Hide resolved
@cawfree cawfree changed the title Introduced parallelism and ignored intermediate build files from jetifier Introduce parallelism for jetifier Jun 19, 2019
bin/jetify Outdated Show resolved Hide resolved
bin/jetify Outdated Show resolved Hide resolved
@mikehardy
Copy link
Owner

Let me start by saying no one should ever feel guilty for not doing open source. That said, I'm curious what your plans are for this? I have to admit, I thought after the second commit, if you just added the ability to override the parallelism max, it was a 10x speedup, which is my general rule of thumb for "ship it" :-). I wonder if an incremental PR with just the first cut might be worth it?

Disclaimer: I ran my rn-androidx-demo project an hour ago and it was so slow I was embarrassed, but everyone is using this repo now 😓

@cawfree
Copy link
Contributor Author

cawfree commented Jun 20, 2019

Hey @mikehardy! Sorry, I got back onto my original project work which was the driver for using this awesome repo. I currently have this snapshot in my package.json.

I'm not convinced that these improvements are even close to what we can get. When I used this cross-platform threading pattern, the performance jumps up wildly, but the files don't appear to be written afterwards. But it seems strange as to why they wouldn't be. I was also doing a little experimentation with gnu-parallel but I wasn't seeing any noticeable improvements for the dependency.

The force push was to undo modifications I made which introduced lossy behaviour again. As it stands, the state of this branch currently works.

I will add support for the parallelism flag but I'd like to continue tinkering with the implementation after that; at least batching the tasks would give us a big gain without draining the resources too quickly... I'll get back to you with a pull request in around an hour.

@mikehardy
Copy link
Owner

Awesome - I saw the lossy behavior when I checked the fancier parallelism version of this PR as well. I'll check this now and if it is stable and faster I'll merge it in - it will be a huge improvement for everyone. And any further work is always welcome, but project work obviously pays the bills ;-). Thanks!

@mikehardy
Copy link
Owner

Some results from a 6-core / 12 vcpu system with SSD and files in RAM (normal after installing packages)

TL;DR:

  • performance is stable after saturation (timing from 12 to 128 on my 12 thread machine), it doesn't degrade, that means it is safe to set parallelism as the default as it didn't hurt performance if it was more workers than cores
  • performance increases pretty linearly up to saturation

So I'm going to set parallelism comfortably above available desktop systems now so people with more hardware get a benefit, without worrying too much about crushing smaller systems since these utilities are pretty small and shouldn't crush CI etc.

npx jetify -w=1

real	1m7.272s
user	2m7.645s
sys	0m25.869s

npx jetify -w=2

real	0m47.687s
user	2m43.196s
sys	0m27.184s

npx jetify -w=3

real	0m45.224s
user	3m5.208s
sys	0m29.034s

npx jetify -w=4

real	0m41.163s
user	3m35.603s
sys	0m30.191s

npx jetify -w=6 (number of actual cores in my machine)

real	0m36.652s
user	4m27.745s
sys	0m33.097s

npx jetify -w=12 (number of virtual cores in my machine)

real	0m32.391s
user	5m14.150s
sys	0m35.695s

npx jetify -w=128 (ridiculous oversaturation / stress)

real	0m32.252s
user	5m13.744s
sys	0m36.101s

@mikehardy
Copy link
Owner

I've merged all the other PR stuff locally and am about to push to this branch then will merge it. Fantastic!

@mikehardy mikehardy changed the title Introduce parallelism for jetifier perf: Introduce parallelism for jetifier Jun 20, 2019
@mikehardy mikehardy merged commit dc547fb into mikehardy:master Jun 20, 2019
@mikehardy
Copy link
Owner

@m4tt72 before making speed claims for the javascript implementation please verify your alternative is substantially faster than the above benchmarks - Having a single solution in a native javascript implementation would be nice and could probably be faster but make sure the numbers are actually there - these timings were all done from the rn-androidx-demo repo with repeated npx jetify runs after the make-demo.sh script had been run once already

@mikehardy
Copy link
Owner

@m4tt72 - check the contributing section if you want to try swapping in the javascript version - it would be neat if it worked, and it should work

There's a test suite and CI now so we can do it safely, and I can make you a contributor on here. I will probably try to move this to the react-native-community umbrella once it's stable, so it can be a general repository - cheers

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.

None yet

3 participants