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

Produce Hermes bytecode and use it on Android #2003

Closed
wants to merge 15 commits into from

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Mar 12, 2020

Fixes #1660

This PR adds the Hermes command line to produce the binary bytecode version of the JS bundle for Android, and sets it as the asset to be loaded by the parent app.

Changes in this PR:

  1. Introduced the bundle:android:bytecode yarn script. Is uses the Hermes binary to compile the App.js bundle into binary bytecode that Hermes' runtime can use to offer better performance on Android. This script is ran automatically as part of the bundle script too.
  2. Have JitPack copy the bytecode file into the Android bridge artifact instead of the App.JS bundle it was bundling before.
  3. Have the glue code load up index.android.bytecode from the assets folder. That's the code the parent app runs to load the bundle so, with this change the parent app will use the binary bytecode file.
  4. Include the binary bytecode file (and the accompanying mapping file) into source control. Note the commited artifacts are not really used by any Android app anyway, only included for "parity" with the string JS bundles being there too.

Comparison on Pixel 2XL Android 10

Without bytecode With bytecode
~4secs loading ~1sec loading
block-editor-open-slow block-editor-open-fast

Also measured the same post opening on a Nexus 5X (Android 7.1.2) and loading takes ~3secs.

Notes

There are more than a few warning emitted by Hermes while compiling the current App bundle into bytecode. Will document here but now sure yet if these are actually important or not. Will probably need to go over these one-by-one to assess:

bundle/android/App.js:36:134: warning: the variable "Promise" was not declared in function "isBoldTextEnabled"

bundle/android/App.js:48:3047: warning: the variable "DebuggerInternal" was not declared in function "value 15#"

bundle/android/App.js:110:57098: warning: the variable "setTimeout" was not declared in function "Pi 1#"

bundle/android/App.js:110:83621: warning: the variable "__REACT_DEVTOOLS_GLOBAL_HOOK__" was not declared in function "Xl"

bundle/android/App.js:110:22512: warning: the variable "clearTimeout" was not declared in function " 657#"

bundle/android/App.js:110:89330: warning: the variable "nativeFabricUIManager" was not declared in function " 692#"

bundle/android/App.js:127:795: warning: the variable "setImmediate" was not declared in function "h 7#"

bundle/android/App.js:137:177: warning: the variable "Worker" was not declared in function " 791#"

bundle/android/App.js:152:67: warning: the variable "fetch" was not declared in function " 855#"

bundle/android/App.js:152:81: warning: the variable "Headers" was not declared in function " 855#"

bundle/android/App.js:152:97: warning: the variable "Request" was not declared in function " 855#"

bundle/android/App.js:152:114: warning: the variable "Response" was not declared in function " 855#"

bundle/android/App.js:153:1539: warning: the variable "FileReader" was not declared in function "p 12#"

bundle/android/App.js:153:1973: warning: the variable "Blob" was not declared in function " 864#"

bundle/android/App.js:153:2034: warning: the variable "FormData" was not declared in function " 864#"

bundle/android/App.js:153:2107: warning: the variable "URLSearchParams" was not declared in function " 864#"

bundle/android/App.js:153:6805: warning: the variable "XMLHttpRequest" was not declared in function " 871#"

bundle/android/App.js:197:245: warning: the variable "MessageChannel" was not declared in function " 993#"

bundle/android/App.js:275:840: warning: the variable "requestAnimationFrame" was not declared in function "value 276#"

bundle/android/App.js:325:2651: warning: the variable "clearImmediate" was not declared in function " 1512#"

bundle/android/App.js:353:1238: warning: the variable "cancelAnimationFrame" was not declared in function "componentWillUnmount 3#"

bundle/android/App.js:886:5988: warning: the property "ldquo" was set multiple times in the object definition.

bundle/android/App.js:1477:12736: warning: the variable "alert" was not declared in function "warn"

bundle/android/App.js:1841:2350: warning: the variable "Intl" was not declared in function "x 32#"

To test:

  1. Use the WPAndroid APK produced on the PR here Use the Gutenberg-mobile binary JS bytecode WordPress-Android#11436 and observe the start-up time of the block editor. It should be faster than before.
  2. Smoke test the editor, it should work as normal.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@hypest hypest marked this pull request as ready for review March 12, 2020 11:40
@maxme
Copy link
Contributor

maxme commented Mar 12, 2020

@rachelmcr can you test the apk linked above on your slower test device(s)?

Note we should also be able to get the exact starting time thanks to the new track property introduced in wordpress-mobile/WordPress-Android#11411

@hypest
Copy link
Contributor Author

hypest commented Mar 12, 2020

Using the new track, it counted 873ms on Pixel 2XL and 2400ms on Nexus 5X.

@rachelmcr
Copy link
Member

can you test the apk linked above on your slower test device(s)?

I tested the new WPAndroid APK on my moto e5 play phone, and it significantly improves the load time for me. I checked the new track property and the load time was between 3124ms and 5513ms each time (out of 5 times) on the moto e5 play. Thank you!

@hypest
Copy link
Contributor Author

hypest commented Mar 19, 2020

Results of manual tests on this PR (cases taken from https://github.com/wordpress-mobile/test-cases/tree/8126e8460ea64bad3e765baec125ae10093d9179/test-suites/gutenberg):

  • Gallery block - Add Gallery Caption - steps

  • Gallery block - Add Gallery Image Caption - steps

  • Gallery block - Close post with an ongoing image upload - steps

  • Gallery block - Choose from device (stay in editor) - Successful upload - steps

  • Gallery block - Choose from device (stay in editor) - Failed upload - steps

  • Gallery block - Choose from device (leave the editor) - Successful upload - steps

  • Gallery block - Choose from device (close and re-open the editor with ongoing uploads) - steps

  • Gallery block - Take a photo - steps

  • Gallery block - Choose from the free photo library - steps ⚠️ Works but on WPAndroid there is a global progress spinner visible before the editor re-appears while the images get added to the post, without individual progress bars in each image.

  • Image block - Switch to classic editor with an image block in page - steps

  • Image block - Add Caption - steps

  • Image block - Insert image from device (failing) - steps

  • Image block - Insert image from device (cancel) - steps

  • Image block - Close/Re-open post with an ongoing image upload - steps

  • Image block - Close post with an ongoing image upload - steps

  • Media Text block - Close post with an ongoing image upload - steps

  • Media Text block - Change media text order during upload and close the post - steps

  • Media Text block - Insert image from device (failing) - steps

  • Media Text block - Insert video from device (failing) - steps

  • Media Text block - Insert image from device (cancel) - steps

  • Media Text block - Insert video from device (cancel) - steps

  • Media Text block - Close/Re-open post with an ongoing image upload - steps

  • Media Text block - Close/Re-open post with an ongoing video upload - steps

  • Simultaneous uploads - steps

Gallery-1

  • Gallery block - Close/Re-open post with an ongoing image upload - steps

Gallery-2

  • Gallery block - Insert image from device (cancel) - steps

Gallery-3

  • Gallery block - Try adding same images from WP Media library and moving the images around - steps

Gallery-4

  • Gallery block - Settings: Link to - steps
  • Gallery block - Settings: Column number - steps
  • Gallery block - Settings: Crop images - steps

MediaText-4

  • Media Text block - Media & Text alignment - steps
  • Media Text block - Vertical alignment - steps

DarkMode-1(iOS)
DarkMode-2(iOS)
Dark Mode on iOS can't be affected by this PR since it's only changing the Android JS bundle.

Shortcode-1

  • Shortcode block - Add a youtube link - steps

Spacer-1

  • Spacer block - Spacer is rendered without crash - steps
  • Spacer block - Settings: Control spacer height using the slider - steps
  • Spacer block - Settings: Control spacer height using the text input - steps
  • Spacer block - Settings: Available height range is correct - steps
  • Spacer block - Settings: Height range extends if Spacer comes from the web is higher than 500px - steps
  • Spacer block - Spacer in horizontal layout works as expected - steps

Button-1

  • Button block - Render custom text color - steps ❌ The editor crashes with Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.. I will update the PR branches and test again.
  • Button block - Render custom background color - steps
  • Button block - Render gradient background color - steps
  • Button block - Check if selection / caret color matches font color - steps

Button-2

  • Button block - Edit text styles - steps
  • Button block - Fallback to default colors in case theme colors are used - steps
  • Button block - New Button is created with the gray color - steps
  • Button block - Settings: Change Button border radius - steps
  • Button block - The newly created button has set background color to the theme-default color in preview - steps
  • Button block - Tapping delete key removes block when Button is empty - steps

Button-3

  • Button block - Settings: Open in new tab - steps
  • Button block - Settings: Link rel - steps
  • Button block - Settings: Link URL - steps
  • Button block - Settings: Remove link - steps
  • Button block - Settings: Synchronize with button options - steps

Button-4

  • Button block - Link in the clipboard is automaticially added into the empty URL field - steps
  • Button block - Toolbar link button is active when Button has link - steps
  • Button block - Button max width is calculated OK inside inner blocks(iOS only) - steps

Group-1

  • Group - AppenderButton is rendered - steps

  • Group - Deep nesting is possible (iOS only) - steps

  • Group - Check if Group placeholder is visible for the unselected state - steps

  • Group - Check if Group placeholder is render in nested structure - steps

  • Group - Nested block have proper border styling - steps

  • Group - Nested block have proper margins values - steps

  • Group - Nested selection cause applying dimmed style on the rest of blocks - steps

  • Group - Breadcrumbs on FloatingToolbar is properly displayed - steps

  • Group - Navigation up button works as expected - steps

  • Group - Navigation down works according to parent-first approach - steps

  • Group - Cross navigation between blocks works as expected - steps

  • Group - Ungroup button works as expected - steps

  • Group - Check if in DarkMode all components gets proper colors (iOS) - steps

  • Group - Check if nested Placeholder block can be replaced - steps

  • Video block - Switch to classic editor with a video block in page - steps

  • Video block - Add Caption - steps

  • Video block - Close post with an ongoing video upload - steps

  • Media Text block - Close post with an ongoing video upload - steps

  • Video block - Insert video from device (failing) - steps

  • Video block - Insert video from device (cancel) - steps

  • Video block - Close/Re-open post with an ongoing video upload - steps

Writing flow

  • Undo-redo (steps)
  • Copy-paste (steps)
  • Splitting blocks (steps)
  • Multiline components (steps)
  • Formatting on rich-text based components (steps) (found an issue with the Strikethrough button)
  • Block insertion (steps)

@maxme
Copy link
Contributor

maxme commented Apr 2, 2020

Undo-redo (steps)

Works fine, but the text cursor is stuck on first character when undoing/redoing text events.

Copy-paste (steps)

Crash everytime I try it :( - doesn't crash on develop (I'll test again in debug mode).

Multiline components (steps)

Stumbled upon an existing issue: #1651

Formatting on rich-text based components (steps) (found an issue with the Strikethrough button)

Stumbled upon an existing issue: #729

@hypest
Copy link
Contributor Author

hypest commented Apr 4, 2020

Reminder to self: The Appium tests are bundling the JS app into the demo app to send it for testing so, should get the bytecode version there too.

@maxme
Copy link
Contributor

maxme commented Apr 6, 2020

Video block - Switch to classic editor with a video block in page - steps
Video block - Add Caption - steps
Video block - Close post with an ongoing video upload - steps

Looks good.

Media Text block - Close post with an ongoing video upload - steps

Got a crash when adding a button block to a media&text block. Note: No crash on current develop

Video block - Insert video from device (failing) - steps
Video block - Insert video from device (cancel) - steps
Video block - Close/Re-open post with an ongoing video upload - steps

Looks good.

@hypest
Copy link
Contributor Author

hypest commented Apr 7, 2020

Copy-paste (steps)

Crash everytime I try it :( - doesn't crash on develop (I'll test again in debug mode).

Indeed, crashes for me as well, with TypeError: Cannot read property 'contains' of undefined. Looks like just by trying to copy paste the picture slider on https://automattic.com/work-with-us/ is enough to cause the crash.

Doesn't crash on SOURCE mode (using Metro). Shall try with binray mode using the string JS app bundle to see what happens there.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 8, 2020

You can trigger optional full suite of UI tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

You can trigger optional full suite of UI tests for these changes by visiting CircleCI here.

@hypest
Copy link
Contributor Author

hypest commented Apr 8, 2020

I've added some (temporary) debugging facility, to quickly switch between the bytecode and Text JS versions of the bundle. To use, add wp.OFFER_GUTENBERG_TEXT_JS_BUNDLE=true in your gradle.settings file and then go into the App Settings in the app to switch between the two options.

What one can immediately see is that Dark Mode doesn't work in the bytecode version. To me, that's an indication of either the two bundles be out of sync or, indeed the bytecode version has various issues atm.

@hypest hypest force-pushed the try/enable-hermes-bytecode branch from e4d7887 to 35e46d6 Compare April 8, 2020 13:14
@hypest
Copy link
Contributor Author

hypest commented Jun 17, 2020

We should also give this experiment a go on RN 0.62 to see if the issues are manifesting there or not.

@maxme maxme removed their request for review October 28, 2020 15:36
@hypest hypest mentioned this pull request Nov 5, 2020
2 tasks
@hypest
Copy link
Contributor Author

hypest commented Nov 9, 2020

The Hermes bytecode improvement has been merged via #2780 so, closing this PR.

@hypest hypest closed this Nov 9, 2020
@hypest hypest deleted the try/enable-hermes-bytecode branch November 9, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use RN's Hermes JS engine for faster and leaner performance on Android
3 participants