-
Notifications
You must be signed in to change notification settings - Fork 47
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
[Hermes] Android integration #410
Conversation
051a078
to
2b42f7a
Compare
9203d45
to
06b3e7b
Compare
/canary |
3 similar comments
/canary |
/canary |
/canary |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
=======================================
Coverage 91.94% 91.94%
=======================================
Files 340 340
Lines 26838 26838
Branches 1946 1946
=======================================
+ Hits 24675 24677 +2
+ Misses 2149 2147 -2
Partials 14 14 ☔ View full report in Codecov by Sentry. |
/canary |
519b090
to
e5f6dde
Compare
06b3e7b
to
bc18da2
Compare
/canary |
2 similar comments
/canary |
/canary |
b237571
to
e1bb8cd
Compare
ec31b1c
to
81061cf
Compare
/canary |
fc0c850
to
64b51a4
Compare
/canary |
1 similar comment
/canary |
/canary |
/canary |
@@ -16,7 +16,7 @@ orbs: | |||
executors: | |||
base: | |||
docker: | |||
- image: docker.io/playerui/bazel-docker | |||
- image: docker.io/playerui/bazel-docker:11 |
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.
Should we merge the docker changes in before merging this so we're not targeting a specific image?
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.
We can! I was avoiding merging that PR until the release was done to make sure it wouldn't accidentally break CI (does make a strong case for tagging our images per release and always targeting a tag).
}, | ||
) | ||
|
||
# TODO: Enable platform support for detecting Android OS as well as cpu |
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.
Will be something we'll have better support for w/ Bazel 7
@@ -120,8 +125,7 @@ use_repo(remote_android_extensions, "android_gmaven_r8") | |||
bazel_dep(name = "rules_jvm_external") | |||
git_override( | |||
module_name = "rules_jvm_external", | |||
# bazel-6 branch | |||
commit = "44f4355b2dbe0d6fd73d690ad66bf5744d482a29", | |||
commit = "73b63ba801f14d1bde7807994cc8c15db226ceec", |
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.
Issue encountered when packaging large binaries:
sugarmanz/rules_jvm_external@73b63ba
@@ -6,4 +6,4 @@ import kotlinx.serialization.Serializable | |||
/** Generic exception for any errors encountered in the scope of the [Player] */ | |||
@Suppress("SERIALIZER_TYPE_INCOMPATIBLE") | |||
@Serializable(ThrowableSerializer::class) | |||
public open class PlayerException(message: String, cause: Throwable? = null) : Exception(message, cause) | |||
public open class PlayerException @JvmOverloads constructor(message: String, cause: Throwable? = null) : Exception(message, cause) |
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.
Necessary to make constructor(message: String)
available to native land
@@ -16,7 +16,7 @@ orbs: | |||
executors: | |||
base: | |||
docker: | |||
- image: docker.io/playerui/bazel-docker | |||
- image: docker.io/playerui/bazel-docker:11 |
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.
We can! I was avoiding merging that PR until the release was done to make sure it wouldn't accidentally break CI (does make a strong case for tagging our images per release and always targeting a tag).
@@ -237,6 +237,7 @@ jobs: | |||
|
|||
- run: | | |||
bazel test --config=ci -- $(bazel query 'kind(".*_test", //...) except filter("ios|swiftui", //...)') -//android/demo:android_instrumentation_test | |||
bazel test --config=ci --//android/player:runtime=hermes -- $(bazel query 'kind(".*_test", //...) except filter("ios|swiftui", //...)') -//android/demo:android_instrumentation_test |
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.
This, plus the one below ensure we re-run all relevant tests (that depend on //android/player:runtime
) against the Hermes runtime. It'll return the cached result for all tests that don't use that config flag, directly or transitively.
@ExperimentalPlayerApi | ||
public fun executeRaw(script: String): Value = | ||
throw UnsupportedOperationException("This experimental method is not implemented for ${this::class.simpleName}") |
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.
New experimental API to reduce serialization cost of executing JS.
@@ -59,4 +64,5 @@ public inline fun <reified T> Runtime<*>.serialize(serializer: SerializationStra | |||
public data class ScriptContext( | |||
val script: String, | |||
val id: String, | |||
val sourceMap: String? = null, |
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.
Powers source map support
# Conflicts: # MODULE.bazel # WORKSPACE.bzlmod
@sugarmanz with this merged can we close #410 and #399? |
Yep! I'll do that in a min! |
Rationale
By default, the Android Player consumes
com.intuit.playerui:j2v8-android
to provide the JS runtime integration since it is reasonably fast and small compared to some of the alternatives (6-7 MB per architecture). However, as the goal with the runtime abstraction was to enable any JS runtime to be swapped out at will, we want to officially support runtimes that bring value to the platform. The Hermes JS runtime created for React Native showed promising size improvements along with a potential runtime performance optimization for pre-compiled JS. Since we also leverage Bazel as our build tool, this empowers us to more easily bring in complex build dependencies as a patternized part of our build. Integrating with this runtime has been on our backlog for several years, and it's great to finally be able to properly consider it.Results
Shown below is a screenshot comparing the APK size of the
//android/demo
application, with some notes beneath:This PR doesn't serve to publish Hermes compatible runtime integrations for non-Android environments, however, it does build Hermes for the host platform to run our JVM
HeadlessPlayer
tests against. J2V8 seemed to have a leg up with regard to desktop performance (without bytecode optimization), but fortunately, Hermes built for Android is different in a few ways and ends up proving to be relatively flat runtime performance in comparison, if not slightly better. More performance testing to come with bytecode optimization.Other improvements include:
Next Steps
The initial integration proved to provide a substantial size decrease, while offering a relatively flat, if not slightly better, runtime performance (on Android). This is a great start, but there are more improvements we can make in future iterations, in order of complexity:
How to review this PR
This is a relatively large PR that includes multiple languages and complex build setup. There will be more comprehensive development docs to come, but here are the key parts to look at:
Bazel setup
host
host
libjsi.so
dependencyhermes-host
for testingOur Bazel setup uses different strategies for integrating Hermes for Android and host machines. For host, it's simple enough to compile Hermes directly and consume for running headless tests. For Android, we have initially used a prebuilt Hermes from React Native - not ideal, but helps us get integrated a bit quicker. We'll want to plan on configuring proper x-compilation for Android so we can have finer grain control of the Hermes build configuration and optimize for our use case. You can see which artifacts are used for which platforms below Android here and host here.
JSI JNI (JavaScript Interface Java Native Interface)
Hermes is built to be a JSI compliant runtime, meaning we can build out our integration with respect to the JSI definition. Some more on this in #395 PR description. More that's not covered in that description includes:
SoLoader
dependency for loading native librariesResourceLoaderDelegate
for host and fallback mechanismJSI Serialization
The Player runtime abstraction is built on the
kotlinx.serialization
, and thus we need an encoder and decoder to support the new JSI types from above. Mostly similar to the other runtimes, leading towards a greater idea of how we can lean into JSI and consolidate the implementations a bit more. Some additional things to note below:RuntimeThreadContext
to explicitly require the correct thread context for accessing thread-unsafe APIsrules_kotlin
patch//jvm/core
Code ChangesMost of the changes in here are small tweaks to fix edge cases I encountered while testing, but there might be some new enhancements to better serve runtime integrations, like
Runtime.executeRaw
andScriptContext.sourceMap
.Change Type (required)
Indicate the type of change your pull request is:
patch
minor
major
Does your PR have any documentation updates?
Release Notes
Initial integration with the Hermes JavaScript runtime. This shows a tremendous size improvement over the existing J2V8 integration of ~70% (7.6 MB -> 2.3 MB, architecture dependent).
Opt-in
For now, the default runtime integration provided by the Android Player will still be
com.intuit.playerui:j2v8-android
, but Hermes can be opted in manually by excluding the J2V8 transitive dependency and including the Hermes artifact:Tip
If your application includes dependencies that may transitively depend on
com.intuit.playerui:android
, you would likely need to ensure the default runtime is transitively excluded from those as well, either manually or as a global strategy.The
AndroidPlayer
will pick the first runtime it finds on the classpath - you can at least verify which runtime was used for thePlayer
with a new log:Player created using $runtime
. But that won't tell you for certain if the other runtimes were successfully excluded. You'll need to examine your APK, or your apps dependency tree, to tell for sure that redundant runtimes aren't unintentionally included.Most of the setup for this integration is done simply by including the right dependency (and excluding the wrong one), however, the
hermes-android
integration also relies on the SoLoader for loading the native libraries. All that's needed is to initialize theSoLoader
(should be on your classpath with thehermes-android
dependency) with an AndroidContext
somewhere before you use theAndroidPlayer
, potentially in your activitiesonCreate
:📦 Published PR as canary version:
0.8.0--canary.410.16288
Try this version out locally by upgrading relevant packages to 0.8.0--canary.410.16288
Version
Published prerelease version:
0.9.0-next.0
Changelog
Release Notes
[Hermes] Android integration (#410)
Initial integration with the Hermes JavaScript runtime. This shows a tremendous size improvement over the existing J2V8 integration of ~70% (7.6 MB -> 2.3 MB, architecture dependent).
Opt-in
For now, the default runtime integration provided by the Android Player will still be
com.intuit.playerui:j2v8-android
, but Hermes can be opted in manually by excluding the J2V8 transitive dependency and including the Hermes artifact:Most of the setup for this integration is done simply by including the right dependency (and excluding the wrong one), however, the
hermes-android
integration also relies on the SoLoader for loading the native libraries. All that's needed is to initialize theSoLoader
(should be on your classpath with thehermes-android
dependency) with an AndroidContext
somewhere before you use theAndroidPlayer
, potentially in your activitiesonCreate
:🚀 Enhancement
Authors: 2