Intl API: JNI initialization, Proguard annotations and intl build definitions. by mganandraj · Pull Request #426 · facebook/hermes

mganandraj · 2020-12-04T19:16:48Z

Summary

This PR has fixes to Intl API implementation to make it work properly under release configurations. With this PR, we also start building the Intl code into hermes release packages.

Essentially, the PR has the following changes,

Add Proguard annotations to keep the JNI referenced Java symbols from being stripped away in minified packages
Ensure fbjni is initialized when loading libhermes.so.
Add runtime config switch to control the inclusion of Intl object in the global. We keep it OFF by default.
Changes in the gradle build definitions to start building C++ and Java Intl code into the hermes release package. We use product flavors to clearly differentiate the Intl enabled packages for now being. We can probably clean things up once the Intl APIs are stabilized.
Updated the documentation (Unfortunately still a docx file, at \lib\Platform\Intl\java\com\facebook\hermes\intl\HermesIntlAPIsImplementationNotes.docx) to include the impact on the size of the application package due to this change.

As the runtime switch is currently turned OFF, the real impact of this change is going to be the increased package size. I'm pasting the measured numbers here for quick reference,

Impact on Application Size
The following numbers are measured using a test application which takes dependency on the Hermes library to evaluate a JavaScript snippet. Essentially, enabling Intl APIs adds 57-62K per ABI.
Product APK Size NOINTL INTL DIFF PERC
ARM64 1,672,235 1,729,579 57,344 3.43%
ARM 1,471,539 1,528,883 57,344 3.90%
X86_64 1,844,255 1,901,599 57,344 3.11%
X86 1,950,739 2,012,179 61,440 3.15%

The overhead is contributed by both compiled native C++ and Java bits.
The uncompressed size of the Hermes shared library got bigger as follows,

libhermes.so Size NOINTL INTL DIFF PERC
ARM64 2,473,760 2,551,592 77,832 3.15%
ARM 1,696,672 1,754,016 57,344 3.38%
X86_64 2,633,528 2,711,368 77,840 2.96%
X86 2,859,916 2,945,936 86,020 3.01%

And the Java bits got bigger as well,
Java Size NOINTL INTL DIFF PERC
classes.jar (in hermes.aar) 559 120975 120,416 21541.32%
classes.dex (intltestapp.apk) 160708 234808 74,100 46.11%

Please note that the application dex file contains non-hermes class files too.
And finally, this is the increase in the final npm package,
NPM Package NOINTL INTL DIFF PERC
214447973 219291220 4,843,247 2.26%

I'm not including the other performance metrics (Memory footprint, latency etc.) with this change because,

Intl APIs are currently disabled by default.
I've measured some metrics and didn't see anything egregious, but I know a couple of potential improvement opportunities, which I want to plug before sharing the numbers, which i'll do as part of enabling Intl by default.

Test Plan

I've a test application which was extremely useful in diagnosing and debugging hermes on Android, which I'll include in another PR.
There is no change in the product code which can affect the functionality, and all the tests are passing.

mhorowitz

Generally, this looks good! One question below.

mhorowitz · 2020-12-12T00:31:26Z

API/hermes/JNI_OnLoad.cpp

@@ -0,0 +1,5 @@
+#include <fbjni/fbjni.h>
+
+extern "C" jint JNIEXPORT JNICALL JNI_OnLoad(JavaVM *jvm, void *reserved) {


This is here presumably to implement "Ensure fbjni is initialized when loading libhermes.so." But this is a bit surprising to me, as hermes.so isn't directly loaded by Java, so I would not think this would be called.

I'm guessing you did this to fix a problem of some sort, but it's not clear what the problem was. Can you say what you were trying to fix with this?

Hi @mhorowitz

As we discussed offline, it was required because hermes is not built to depend on libfbjni.so, but instead directly compiling the fbjni c++ sources into the libhermes.so. This results in libhermes.so having a private copy of the jvm pointer.

But, later when I tried with react-native, the fbjni calls to Java code seems to work even without this initialization. Hence, there is something going on which i haven't understood well enough.

Please let me know if you want me to try to look into the details here.

@mganandraj I think your observation about libhermes including its own fbjni is on point and relevant. I think the correct long-term fix is for Hermes to depend on prebuilt external fbjni from maven as RN does, so they can both share the same libfbjni.so. But this is not a simple fix, so for now, including JNI_OnLoad in libhermes is an ok workaround, but we should try to fix this for real, too.

facebook-github-bot

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mhorowitz · 2020-12-18T03:03:38Z

Trying to land, I'm getting a lot of errors like

/tmp/tmp.Xk98BTFios/hermes/lib/Platform/Intl/java/com/facebook/hermes/intl/NumberFormat.java:15: error: package com.facebook.proguard.annotations does not exist
import com.facebook.proguard.annotations.DoNotStrip;
                                        ^

which makes sense as hermes doesn't have com.facebook.proguard. I'm not quite sure how to fix this.

Our internal tests run

$GRADLE -Pabis=x86,arm64-v8a hermes:build
$GRADLE -Pabis=armeabi-v7a build intl:build
$GRADLE -Pabis=x86_64 build intltest:build

And it appears the failure is on the second command. Hopefully you can repro the failure with that.

mganandraj · 2020-12-18T04:17:43Z

Trying to land, I'm getting a lot of errors like
/tmp/tmp.Xk98BTFios/hermes/lib/Platform/Intl/java/com/facebook/hermes/intl/NumberFormat.java:15: error: package com.facebook.proguard.annotations does not exist
import com.facebook.proguard.annotations.DoNotStrip;
                                        ^
which makes sense as hermes doesn't have com.facebook.proguard. I'm not quite sure how to fix this.

Our internal tests run
$GRADLE -Pabis=x86,arm64-v8a hermes:build
$GRADLE -Pabis=armeabi-v7a build intl:build
$GRADLE -Pabis=x86_64 build intltest:build
And it appears the failure is on the second command. Hopefully you can repro the failure with that.

The DoNotStrip is brought in through dependency to "com.facebook.yoga:proguard-annotations:1.14.1" ..

"intl" project has this dependency defined but is under "androidTestImplementation" which seems to be causing issues here, as it may not take effect in non-test builds .. I've pushed another update removing bulding "intl" project as it is no longer needed.. Also I've cleaned up the remaining linter errors.

facebook-github-bot · 2020-12-18T04:51:42Z

@mganandraj has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mhorowitz · 2020-12-18T06:45:40Z

https://github.com/facebook/hermes/pull/426/files/3c667a277f3f8518c98f277d3b4d275aa4dfc147..f30044da909a700241ba536605ab541f8b98871d shows changes besides the ones we discussed.

Some of these changes are obvious, but some are not. Can you remove the changes which aren't strictly necessary to fix the land failures? We can always make more changes later, but I'd prefer to avoid creeping changes into a PR this late without some discussion of why. This risks slowing down the process, and I only have one more work day left this year.

facebook-github-bot · 2020-12-18T16:52:52Z

@mganandraj has updated the pull request. You must reimport the pull request before landing.

mganandraj · 2020-12-18T16:59:01Z

Some of these changes are obvious, but some are not. Can you remove the changes which aren't strictly necessary to fix the land failures? We can always make more changes later, but I'd prefer to avoid creeping changes into a PR this late without some discussion of why. This risks slowing down the process, and I only have one more work day left this year.

Almost all changes are purely for silencing linter .. None are necessary (Made supposedly to make the pull smoother), hence removed all changes other than removing "intl" module from building.

facebook-github-bot

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-12-18T23:34:51Z

@mhorowitz merged this pull request in c3fab01.

Huxpro · 2021-03-26T21:40:14Z

lib/VM/JSLib/GlobalObject.cpp

-      normalDPF,
-      intl::createIntlObject(runtime)));
+
+  if (LLVM_UNLIKELY(runtime->hasES6Intl())) {


Hey @mganandraj, I'm a little confused by the purpose of having this runtime config. My understanding was that Intl is mostly a compile-time option. If Hermes lib was already compiled with the Intl flag on, (say for the next OSS release), then the binary size is already increased. Does unsetting this runtime config help with any savings?

Maybe you did some measurement on the TTRC and found out that the cost of initializing Intl object alone is worrying?

EDIT: I discussed with Marc and we are okay of having it so people can turn it off even they are not compiled from source. That being said, it would be still useful to hear what's in your mind.

Chaidez5 · 2021-05-25T13:57:06Z

Development of the world

mganandraj added 2 commits December 3, 2020 22:09

JNI initialization, Proguard annotations and intl build definitions.

3e70f84

Adding documentation

e6c7b75

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 4, 2020

Enabling Intl flag in the intl tests

3c667a2

mganandraj mentioned this pull request Dec 7, 2020

A test application for Hermes on Android and some cleanups #427

Closed

mhorowitz reviewed Dec 12, 2020

View reviewed changes

facebook-github-bot reviewed Dec 12, 2020

View reviewed changes

facebook-github-bot reviewed Dec 18, 2020

View reviewed changes

Removing outdated module 'intl'

6206cdf

mganandraj force-pushed the IntlRelease2 branch from f30044d to 6206cdf Compare December 18, 2020 16:52

facebook-github-bot reviewed Dec 18, 2020

View reviewed changes

facebook-github-bot closed this in c3fab01 Dec 18, 2020

mganandraj deleted the IntlRelease2 branch December 18, 2020 23:16

facebook-github-bot added the Merged label Dec 18, 2020

Huxpro reviewed Mar 26, 2021

View reviewed changes

mhorowitz mentioned this pull request May 27, 2021

Intl Support #23

Closed

		@@ -0,0 +1,5 @@
		#include <fbjni/fbjni.h>

		extern "C" jint JNIEXPORT JNICALL JNI_OnLoad(JavaVM jvm, void reserved) {

Conversation

mganandraj commented Dec 4, 2020

Summary

Test Plan

Uh oh!

mhorowitz left a comment

Choose a reason for hiding this comment

Uh oh!

mhorowitz Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

mganandraj Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

mhorowitz Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

mhorowitz commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mganandraj commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Dec 18, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

mhorowitz commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Dec 18, 2020

Uh oh!

mganandraj commented Dec 18, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 18, 2020

Uh oh!

Huxpro Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chaidez5 commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mhorowitz commented Dec 18, 2020 •

edited

Loading

mganandraj commented Dec 18, 2020 •

edited

Loading

mhorowitz commented Dec 18, 2020 •

edited

Loading

Huxpro Mar 26, 2021 •

edited

Loading