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

Another alternative #21

Closed
mgtitimoli opened this issue Jun 26, 2019 · 11 comments
Closed

Another alternative #21

mgtitimoli opened this issue Jun 26, 2019 · 11 comments

Comments

@mgtitimoli
Copy link

mgtitimoli commented Jun 26, 2019

Hi @m4tt72,

I really appreciate the effort you spent on this tool, last week I integrated with our flow and it allowed us to unlock the issue we were experiencing with some of the deps we have that they haven't migrated to AndroidX yet, but right after we did this, I had to start working on an alternative that could finish the process in less than 5m as this jetifier was taking more than 15m and this was preventing the CI to finish the build.

After spending a couple of days on it, I was finally able to release it publicly Yesterday:

@jumpn/react-native-jetifier

By maintaining an index with all the jetificables under node_modules, and jetifying all of them in parallel time went down from 15m to 8s on the first run (without index), and 1.5s in the following ones (with index).

Thanks again for having open sourced this tool that served me as inspiration to create @jumpn/react-native-jetifier, whenever you have same time, give it a try, it's needless to say that I will very well welcome any feedback :).

@mikehardy
Copy link
Owner

Actually, it was my version that was slow, @m4tt72 implemented a node version that was very fast, and with his collaboration I integrated it here yesterday as 1.5.0 - you might try the newest version in fact and see that it is fast enough, without having to maintain state in the form of an index? If it is in fact fast enough then I can only say I am sad my first version wasn't fast enough to result in duplicated effort. This AndroidX thing was sort of a tsunami and resulted in so much lost time everywhere...

@mikehardy
Copy link
Owner

That said, if there's a possibility of safely (like: safe every single time - I fear state in build systems because then there always seems to be a need for a "cleaning" tool) integrating an index to save work on postinstall I'm always open to collaboration

@mgtitimoli
Copy link
Author

mgtitimoli commented Jun 26, 2019

The main purpose of the index is to keep a list of all the jetificable files present on each of the packages under node_modules. Each time the tool runs it gets all your packages and compare name and version of them against the one in the index, if it's the same, then it jetifies the list of files described there otherwise, it searches for jetificables, update the index, and then it jetifies them.

@mgtitimoli
Copy link
Author

Having an index reduce a bit the time taken to process, but the main boost on performance was done by processing all the files in parallel, this reduced the time from 15m to 8s.

@mikehardy
Copy link
Owner

🤔 I think I get it. I just fear things that “keep a list” - I am getting old, and I have so many battle scars from list keeping :-). I would be curious to see a benchmark of the current 1.5.x release line of jetifier vs react-native-jetifier. For performance optimization I’m usually looking for big speedups and I’m not sure there is much more performance to gain vs the current implementation. In Travis CI the current 1.5.x jetifier handles a reasonable group of dependencies in just 2-3 seconds, which for me is inconsequential vs the time a “yarn install” needs to run anyway, so I no longer have a human-level feeling that it’s an impact, and CI doesn’t choke either (important also).

The previous version though, oh yes - that was slow!

@mikehardy
Copy link
Owner

The rn-androidx-demo project is what I use as a test suite for these things btw - you could try it? Once it’s done the general install you can just use shell time npx jetify to see vs time npx react-native-jetifier

@mgtitimoli
Copy link
Author

mgtitimoli commented Jun 26, 2019

I've just run both on our app

Tool First Run Second Run After Reinstall
jetifier 5.031s 4.834s 4.972s
@jumpn/react-native-jetifier 11.282s 1.232s 1.245s

With these times we can see the impact of having an index that adds some secs to the first run, so running slower than jetifier, and in the following executions is what makes react-native-jetifier faster.

@mikehardy
Copy link
Owner

Interesting - undeniably faster. I don’t find 5s to be terrible on something I don’t do as part of my normal test/code cycle but that’s a real speedup.

Some thoughts -

I am still very wary about adding state to any system. Under what circumstances could the state become incorrect vs the on-disk reality of the process? What if people use patch-package? Or even some intentional hand-hacking inside node_modules (a terrible idea, but just to think about it). And how would exactly would state be removed, does the tool itself support state removal?

Why does index construction take so much time? Seems odd that index construction takes twice as much time as a full translation of the jetify@1.5.x ?

Would you be willing to PR an index-boosted version of the existing jetify that people could toggle on?

My vision for this repository is to encourage rapid development of a rock-solid utility in preparation for react-native 0.60 (which will be AndroidX), then likely move it to react-native-community if they’ll hold it (I have another couple repos there). I believe the bulk of the work over the summer will be creating PRs for react-native libraries to fix them, and user support, so it won’t be glorious :-), but I am open to committed collaborators via github and npm permissions if it’s interesting

@mgtitimoli
Copy link
Author

Index only contains the jetificable paths grouped by package, so unless people add more jetificables manually to a package, then it will always work.

Creating the Index might take more time because it first retrieves all package.json, and then it analizes all the java/kotlin/xml files on each of them and saves the path of the ones that are jetificable. There could be another ways of doing this, like for example look for jetificables and then generate the path of all the package.json after having grouped them based on their location, so this way you will be able to skip the glob used to get all package.json.

Regarding of submitting a PR, I initially was gonna do this, but as soon as I realize I had to do a full rewrite then I took the other path but I would be totally cool if you want to take the one I did and push it to react-native-community.

@mikehardy
Copy link
Owner

I’ll think about maybe maintaining an index if there are performance complaints, but the testing and documentation and support and user outreach related to this package is the overwhelming amount of the work. The code is of course vital but almost an afterthought vs the rest, so I’ll let it sit for now. Never a bad thing to have multiple options though :-). Cheers @mgtitimoli

@mikehardy
Copy link
Owner

with pull request #24 here we've got a 4x speedup on initial and subsequent runs - roughly equivalent to your indexed speedup - from parallelization without state. I don't mention that to be irritating! I have no doubt after index construction that if that idea was layered on it would be faster yet, but I do mention it because usually when I have two choices with equivalent performance and one doesn't involve me working up my own solution, I like to know :-). 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

No branches or pull requests

2 participants