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.

Sign up for GitHub

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

Jump to bottom

Use Hermes bytecode on Android #2780

Merged
merged 6 commits into from
Nov 6, 2020
Merged

Use Hermes bytecode on Android #2780

merged 6 commits into from
Nov 6, 2020

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Nov 5, 2020 •
edited
Loading

Fixes #1660

This PR is a continuation of #2003, with a simpler approach and just using the bytecode version directly. If it proves more successful I will close the other PR.

Gutenberg PR: WordPress/gutenberg#26732
WPAndroid PR: wordpress-mobile/WordPress-Android#13297

I've left a comment with the (green!) results of my Writing Flow manual tests here.

Changes in this PR

  • Use the bytecode version of the JS bundle for the apps and the tests

To test:

  • No special testing other than all operations should look and feel faster.
  • A perhaps easier to spot improvement is the editor startup time. Use the WPAndroid PR/build to start a new blog post and notice how much faster the editor comes online.

PR submission checklist:

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

@hypest hypest changed the title Gutenberg/use hermes bytecode nov 2020 Use Hermes bytecode on Android Nov 5, 2020
@hypest hypest mentioned this pull request Nov 5, 2020
Closed
3 tasks
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 5, 2020 •
edited
Loading

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@mchowning
Copy link
Contributor

mchowning commented Nov 5, 2020 •
edited
Loading

Just noting that since these changes result in new bundle/android/App.text.js and bundle/android/App.text.js.map files, we should just make sure we generate and add those files to this PR before we merge it. I realize this comment is premature (it's only a draft PR after all), but I just wanted to note it so we didn't forget.

@hypest
Copy link
Contributor Author

hypest commented Nov 6, 2020

Just noting that since these changes result in new bundle/android/App.text.js and bundle/android/App.text.js.map files, we should just make sure we generate and add those files to this PR before we merge it. I realize this comment is premature (it's only a draft PR after all), but I just wanted to note it so we didn't forget.

Not premature at all, thanks for the comment @mchowning! Not sure though, since we are not keeping the bundle files under source control anymore, what do you mean by generating and adding those files to this PR? For what is worth, I already added them to .gitignore, same as the non ...text... ones.

@mchowning
Copy link
Contributor

since we are not keeping the bundle files under source control anymore, what do you mean by generating and adding those files to this PR? For what is worth, I already added them to .gitignore, same as the non ...text... ones.

I forgot that we're not committing the files anymore. Adding them to .gitignore is perfect. 👍

@hypest hypest force-pushed the gutenberg/use-hermes-bytecode-nov-2020 branch from ee4c1b0 to fd80fe6 Compare November 6, 2020 15:33
@hypest
Copy link
Contributor Author

hypest commented Nov 6, 2020

I rebased this PR to remove the GB updates that are by now merged into develop anyway and are not related to this PR.

@hypest hypest requested a review from mchowning November 6, 2020 16:02
@hypest hypest added this to the 1.41 milestone Nov 6, 2020
@hypest hypest marked this pull request as ready for review November 6, 2020 16:02
mchowning
mchowning reviewed Nov 6, 2020
View reviewed changes
package.json
"test:e2e:bundle:android": "mkdir -p gutenberg/packages/react-native-editor/android/app/src/main/assets && npm run rn-bundle -- --reset-cache --platform android --dev false --minify false --entry-file index.js --bundle-output gutenberg/packages/react-native-editor/android/app/src/main/assets/index.android.bundle --assets-dest gutenberg/packages/react-native-editor/android/app/src/main/res",
"test:e2e:bundle:android": "npm run test:e2e:bundle:android:text && npm run test:e2e:bundle:android:bytecode",
"test:e2e:bundle:android:text": "mkdir -p gutenberg/packages/react-native-editor/android/app/src/main/assets && npm run rn-bundle -- --reset-cache --platform android --dev false --minify false --entry-file index.js --bundle-output gutenberg/packages/react-native-editor/android/app/src/main/assets/index.android.text.bundle --assets-dest gutenberg/packages/react-native-editor/android/app/src/main/res",
"test:e2e:bundle:android:bytecode": "./gutenberg/node_modules/hermes-engine/`node -e \"const platform=require('os').platform();console.log(platform === 'darwin' ? 'osx-bin' : (platform === 'linux' ? 'linux64-bin' : (platform === 'win32' ? 'win64-bin' : 'unsupported-os')));\"`/hermes -emit-binary -O -out gutenberg/packages/react-native-editor/android/app/src/main/assets/index.android.bundle gutenberg/packages/react-native-editor/android/app/src/main/assets/index.android.text.bundle -output-source-map",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue for this PR, but it feels like we should start extracting these out into some separate script files for the sake of readability. That might even open up some possibilities for reusing some of our npm script "logic" both here and in gutenberg/packages/react-native-editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯
Non trivial command executions and pipelines, plus very long pathnames are impossible to read. I'd like to work on that, possibly even next week. Please, feel free to also share ideas on how to reuse logic 👍 !

mchowning
mchowning approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested these changes in the demo app and WPAndroid and everything is working great.

@mchowning mchowning merged commit 1013d68 into develop Nov 6, 2020
@mchowning mchowning deleted the gutenberg/use-hermes-bytecode-nov-2020 branch November 6, 2020 22:39
@ceyhun ceyhun mentioned this pull request Nov 9, 2020
Merged
4 tasks
@hypest hypest mentioned this pull request Nov 9, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchowning mchowning mchowning approved these changes

Assignees

@mchowning mchowning

Labels
Performance
Projects
None yet
Milestone
1.41 (16.2)
Development

Successfully merging this pull request may close these issues.

Use RN's Hermes JS engine for faster and leaner performance on Android
2 participants
@hypest @mchowning