Skip to content
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

[Feature] Hot code reloading #184

Open
1 task
jmgao opened this issue Apr 20, 2022 · 6 comments
Open
1 task

[Feature] Hot code reloading #184

jmgao opened this issue Apr 20, 2022 · 6 comments
Assignees

Comments

@jmgao
Copy link
Contributor

jmgao commented Apr 20, 2022

Background

Iteration time is everything. It would be really, really cool to be able to update code and have it reload in real-time

Options

TODO

@jmgao jmgao added the maintenance A maintenance task! label Apr 20, 2022
@kanerogers
Copy link
Collaborator

This sounds brilliant, @jmgao! Do you've any thoughts on implementation for this?

A viable idea might be to have something like hotham-debug-runner that could watch the target directory and replace the library on build. That way, a user could just run cargo build (with the appropriate targets etc, which we could package up into a nice script, or even our own cargo command!) and the watcher service could adb push them into the app directory and restart the app.

@jmgao
Copy link
Contributor Author

jmgao commented Apr 22, 2022

I did some cursory investigation, and the way Android loads the NativeActivity makes things a bit annoying to do with ndk-glue as is, because we're loading the entire library before we have a chance to load the library from another location.

I think the easiest solution is to do the following:

  • send a patch to cargo-apk to allow us to override the library name used for the native activity in the AndroidManifest.xml
  • use that to point Android at a libhotham_loader.so, not sure what the best way to build this is
  • libhotham_loader.so loads either libraries from a fixed path in the data directory, or the lib directory, or the apk (not sure how we should decide: either bake it into libhotham_loader, or decide dynamically based on the package name? presumably we don't want to stick with rust.<name of library> as the only possible package name long term, and cargo-apk has a patch merged (but not released yet afaik) that lets you change the package name)
  • bundle a script that does a warm reload by pushing and restarting the app. I think a watcher service is overkill, given that cargo watch already exists: you can just do cargo watch -x build -s "./scripts/reload.sh"

Now that I type this, I vaguely remember Android gaining an selinux restriction that prohibits loading libraries from application data directories if you have a sufficiently new target SDK version, but I don't remember if that happened before or after the version that the quest is on, but there are hacks around that...

@jmgao
Copy link
Contributor Author

jmgao commented Apr 22, 2022

Now that I type this, I vaguely remember Android gaining an selinux restriction that prohibits loading libraries from application data directories if you have a sufficiently new target SDK version, but I don't remember if that happened before or after the version that the quest is on, but there are hacks around that...

I remembered right, but it lasted for 9 days in the tree before it was reverted.

@kanerogers
Copy link
Collaborator

I did some cursory investigation, and the way Android loads the NativeActivity makes things a bit annoying to do with ndk-glue as is, because we're loading the entire library before we have a chance to load the library from another location.

I think the easiest solution is to do the following:

  • send a patch to cargo-apk to allow us to override the library name used for the native activity in the AndroidManifest.xml
  • use that to point Android at a libhotham_loader.so, not sure what the best way to build this is
  • libhotham_loader.so loads either libraries from a fixed path in the data directory, or the lib directory, or the apk (not sure how we should decide: either bake it into libhotham_loader, or decide dynamically based on the package name? presumably we don't want to stick with rust.<name of library> as the only possible package name long term, and cargo-apk has a patch merged (but not released yet afaik) that lets you change the package name)
  • bundle a script that does a warm reload by pushing and restarting the app. I think a watcher service is overkill, given that cargo watch already exists: you can just do cargo watch -x build -s "./scripts/reload.sh"

Now that I type this, I vaguely remember Android gaining an selinux restriction that prohibits loading libraries from application data directories if you have a sufficiently new target SDK version, but I don't remember if that happened before or after the version that the quest is on, but there are hacks around that...

Oooh, that does sound like an interesting implementation.

My gut reaction is this feels like overkill and a potential source of tricky bugs. That said, if it'll give us a better developer experience, it may be worth it!

What would the indirection of adding a libhotham_loader.so give us, that couldn't be accomplished by replacing the library and restarting the app? From my experience the time to restart the app is pretty minimal, and it definitely feels like it could be a lot cleaner / simpler.

@jmgao
Copy link
Contributor Author

jmgao commented Apr 22, 2022

What would the indirection of adding a libhotham_loader.so give us, that couldn't be accomplished by replacing the library and restarting the app? From my experience the time to restart the app is pretty minimal, and it definitely feels like it could be a lot cleaner / simpler.

You can't replace the library without rebuilding the APK and installing it: the application manifest says to load libcrab_saber.so, and Android will load that either from the read-only lib directory, or directly from the APK. Rebuilding the APK takes 14 seconds for me (?!?!?! why is it taking so long??) and installing takes ~4 seconds. Just pushing the libraries should be pretty much instantaneous (adb push goes at 200MB/s on my Quest 2, and that's going to get faster eventually)

We need a layer of indirection because we don't want to actually load the application library until we know which one to load, because there might be static initializers that don't expect to run twice in the same process, symbol collision if NativeActivity loads with RTLD_GLOBAL, and probably other horrible undebuggable landmines.

@kanerogers kanerogers self-assigned this Feb 23, 2023
@kanerogers kanerogers changed the title [Feature] Faster iteration by avoiding apk creation/installation [Feature] Hot Reloading Feb 23, 2023
@kanerogers
Copy link
Collaborator

Original comment from @jmgao

It takes a pretty long time to build the libraries, create an apk and install the apk. We can improve on this by checking for libraries in a writable location and loading those over the ones in the apk if found. Then we can build just the libraries, push those to the app data directory and then restart the app.

(I think we can improve on this further by making hotham a separate shared library from the actual application, but the cargo incantations for that might be painful)

@kanerogers kanerogers changed the title [Feature] Hot Reloading [Feature] Hot code reloading Feb 23, 2023
@kanerogers kanerogers added developer-experience and removed maintenance A maintenance task! labels Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants