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

Optimise the size of iOS artifacts #368

Closed
grabbou opened this issue Sep 22, 2020 · 17 comments
Closed

Optimise the size of iOS artifacts #368

grabbou opened this issue Sep 22, 2020 · 17 comments
Assignees

Comments

@grabbou
Copy link
Contributor

grabbou commented Sep 22, 2020

Problem

Right now, the artifact itself is rather large after all architectures, dSYMs, and bitcode were included - 2GB. We need to find a way to reduce this to a reasonably small size, so that it doesn't affect React Native users.

The goal is to reduce the size so that it comes w/o penalty for new React Native applications.

Solution

  • Optimise size of hermes-engine-darwin npm package
    • Turn off bitcode (2GB with bitcode enabled for three architectures, 12MB without bitcode). While it saves the size, it makes it impossible to build React Native with bitcode, which comes "on" by default. Potential change in the default template possible as long as it makes sense to do so. We can also disable bitcode for iphonesimulator architectures as I think it makes no sense to ship that for non-release architectures (won't be able to build for simulator in the release tho)
    • Decide what to do with symbols - dSYM weights 200mb
  • Make it optional package, so that users will have to install hermes-engine-darwin npm package on demand (unlike for Android) - size constraint doesn't affect the initial setup time. Potential degraded DX as it requires many actions to be done to enable Hermes
  • Do not use npm for hermes-engine-darwin, but use CocoaPods dependency management mechanism. We can publish full Hermes to CocoaPods repository and it will be automatically installed as soon as React Native users change the flag in a Podfile to enable_hermes. This is done by removing path to a Hermes podspec that points to node_modules.
  • Build Hermes in the user land (don't ship artifacts)

Opening this issue to discuss potential ways going forward and options that we have.

@alloy
Copy link
Contributor

alloy commented Sep 22, 2020

We can also disable bitcode for iphonesimulator architectures as I think it makes no sense to ship that for non-release architectures (won't be able to build for simulator in the release tho)

We definitely do not need bitcode for the simulator. IIRC, Xcode doesn’t even actually build bitcode until an archive release is built.

Are you saying Xcode won’t allow a simulator release build without the slice present?

Decide what to do with symbols

Symbols should be made available, at the very least for release builds in order to symbolicate crash reports. These could potentially be made available separately for when needed, but we have to take into account the possibility that RN users will not know/remember to do so and in turn struggle with unhelpful crash reports.

Do not use npm for hermes-engine-darwin, but use CocoaPods dependency management mechanism

I am in favour of not using npm. At first it seemed like it might be good to stay consistent for now, but it’s becoming clear that it’s only adding friction, with eg mow needing to decide when to install it.

Make it optional package, so that users will have to install hermes-engine-darwin npm package on demand

Strong yes on this from me.

FWIW I believe we also shouldn’t install the Android runtime by default and perhaps also not via npm.

Build Hermes in the user land

This would indeed solve the artefact size issue, but the build isn’t trivial to convert (although everything is doable) and would increase the build time of RN projects.

@grabbou
Copy link
Contributor Author

grabbou commented Sep 22, 2020

Are you saying Xcode won’t allow a simulator release build without the slice present?

I believe when bitcode is turned on and simulator slices don't contain bitcode, the build will fail, unless you turn that off for that particular build. I am fine with that, but I am trying to think for the users as well.

My current preferred solution is to release Hermes to the CocoaPods repository with artifacts and have pod install automatically download required artifacts (no matter their size) as soon as users flip the hermes_enabled flag and re-run installation.

I think this is totally fine, given it's what Flipper does already here. I think the approach of using npm makes sense for smaller packages, such as community native modules. For a package like this one, where 100% is native code, I don't see any benefits of going that path.

@mhorowitz
Copy link
Contributor

2gb! This sounds painful for everyone without the biggest network connections.

How does the CocoaPods repository release process work?

@alloy
Copy link
Contributor

alloy commented Sep 23, 2020

@mhorowitz CocoaPods isn’t a package manager, so it doesn’t host packages; this means we just need to serve the artefacts from somewhere, e.g. a GitHub release, and then point the CocoaPods podspec to that location.

CocoaPods also supports tarballs compressed with xz, so I’m interested in seeing how large the archive will be when compressed with that algorithm.

@grabbou
Copy link
Contributor Author

grabbou commented Sep 23, 2020

I will experiment with that and post results at the beginning of next week.

@grabbou
Copy link
Contributor Author

grabbou commented Sep 28, 2020

I am back after holidays, resuming my work on this one. Will keep you posted.

The plan here is to not build bitcode for non-release targets (simulator) and to ship to CocoaPods. That should offload the installation of Hermes until it is turned on via a Podfile.

@alloy
Copy link
Contributor

alloy commented Oct 1, 2020

To publish a podspec to the centralised spec repo, you need to follow the steps outlined here: https://guides.cocoapods.org/making/getting-setup-with-trunk.html

An important thing to note here is that, as we’re planning to distribute prebuilt binaries, the source attribute of the spec needs to be updated for each release and we can leave out the prepare_command entirely. It’s probably easiest to look for an env variable from within the podspec, e.g.:

spec.source = ENV['hermes-artefact-url'] ? ENV['hermes-artefact-url'] : { git: "https://github.com/facebook/hermes.git", tag: "v#{spec.version}" }

unless ENV['hermes-artefact-url']
  spec.prepare_command = '…'
end

And then do:

env hermes-artefact-url='' pod trunk push hermes.podspec

@mhorowitz
Copy link
Contributor

Thanks, @alloy, a couple more questions.

  1. If we push a podspec with ENV checks for the binary artifact, won't the user of the pod need to set the env var in order to download the prebuilt version? Is this something the RN podspec would do?

  2. How does versioning work? In the future when we create v0.8.0 for use by rn 0.65, how do we have rn 0.64 continue to use hermes v0.7.x? Is there any mechanism for automatic updates to new patch releases?

@alloy
Copy link
Contributor

alloy commented Oct 1, 2020

  1. When you push a podspec it will be serialized to JSON, so the evaluation results are then recorded and won’t require the user to do anything.

  2. Setting version requirements on “pods” works similarly as with eg npm. That is, the React-Core/Hermes module in the React-Core.podspec in RN will have a requirement such as ~> 0.7, which indicates any 0.7.x (preferably the latest possible) will be used.

facebook-github-bot pushed a commit that referenced this issue Oct 5, 2020
Summary:
Bitcode is not needed for `iphonesimulator` - this PR makes bitcode to be included with iOS only. Reduces the package size by ~1GB.

Related to #368

Pull Request resolved: #376

Reviewed By: willholen

Differential Revision: D24031684

Pulled By: Huxpro

fbshipit-source-id: 342b0139d208cdea6c2af2983c894b758a7d7789
Huxpro pushed a commit that referenced this issue Oct 8, 2020
Summary:
Bitcode is not needed for `iphonesimulator` - this PR makes bitcode to be included with iOS only. Reduces the package size by ~1GB.

Related to #368

Pull Request resolved: #376

Reviewed By: willholen

Differential Revision: D24031684

Pulled By: Huxpro

fbshipit-source-id: 342b0139d208cdea6c2af2983c894b758a7d7789
Huxpro added a commit that referenced this issue Oct 8, 2020
This PR did two things.

1. rename to `hermes-engine` since <https://cocoapods.org/pods/Hermes> already exists
2. conditionalize `spec.source` so it can distribute prebuilt binaries as @alloy suggested at #368 (comment)
Huxpro added a commit that referenced this issue Oct 8, 2020
This PR did two things.

1. rename to `hermes-engine` since <https://cocoapods.org/pods/Hermes> already exists
2. conditionalize `spec.source` so it can distribute prebuilt binaries as @alloy suggested at #368 (comment)
facebook-github-bot pushed a commit that referenced this issue Oct 9, 2020
Summary:
This PR did two things.

1. rename everything to `hermes-engine` since <https://cocoapods.org/pods/Hermes> already exists
2. conditionalize `spec.source` so it can distribute prebuilt binaries as alloy suggested at #368 (comment)

Pull Request resolved: #385

Test Plan: CircleCI doing fine

Reviewed By: tmikov

Differential Revision: D24206180

Pulled By: Huxpro

fbshipit-source-id: ec46350e00099c61a7de8cc9eb99991af72abdb4
Huxpro added a commit that referenced this issue Oct 9, 2020
Summary:
I cherry-picked this PR which:

1. rename everything to `hermes-engine` since <https://cocoapods.org/pods/Hermes> already exists
2. conditionalize `spec.source` so it can distribute prebuilt binaries as alloy suggested at #368 (comment)

Noted that the version in the `podspec` had been updated to 0.7.1
correctly.

Pull Request resolved: #385

Test Plan: CircleCI doing fine

Reviewed By: tmikov

Differential Revision: D24206180

Pulled By: Huxpro

fbshipit-source-id: ec46350e00099c61a7de8cc9eb99991af72abdb4
@Huxpro
Copy link
Contributor

Huxpro commented Oct 10, 2020

@alloy it seems like pod dislike spec.source being a String and asked for a Hash?

$ env hermes-artifact-url='https://github.com/facebook/hermes/releases/download/v0.7.1/hermes-engine-darwin-v0.7.1.tgz' pod trunk push hermes-engine.podspec
Updating spec repo `trunk`

Validating podspec
 -> hermes-engine (0.7.1)
    - ERROR | source: Unsupported type `String`, expected `Hash`
    - ERROR | unknown: Encountered an unknown error (no implicit conversion of Symbol into Integer) during validation.

[!] The spec did not pass validation, due to 2 errors.

Btw, I realized pod is case sensitive via pod trunk info, so technically we can still use hermes (it was Hermes being taken LOL). Not sure if that's a good idea and better than hermes-engine though.

@alloy
Copy link
Contributor

alloy commented Oct 11, 2020

Ah yes indeed, my bad. See the doc on it here https://guides.cocoapods.org/syntax/podspec.html#source. The hash can also include a checksum for the artefact.

@alloy
Copy link
Contributor

alloy commented Oct 11, 2020

As for the name, I think hermes-engine would probably be better to entirely disambiguate from the existing Hermes pod.

@Huxpro
Copy link
Contributor

Huxpro commented Oct 12, 2020

@alloy the current document seem to use a different syntax (:git => ...), but from what I understood I changed the podspec to be:

spec.source = ENV['hermes-artifact-url'] ? { http: ENV['hermes-artifact-url'] } : { git: "https://github.com/facebook/hermes.git", tag: "v#{spec.version}" }

then redo the push. But this is what I got:

λ env hermes-artifact-url='https://github.com/facebook/hermes/releases/download/v0.7.1/hermes-engine-darwin-v0.7.1.tgz' pod trunk push hermes-engine.podspec

Validating podspec
... (warnings) ...
 -> hermes-engine (0.7.1)
    - ERROR | file patterns: The `source_files` pattern did not match any file.
    - ERROR | file patterns: The `preserve_paths` pattern did not match any file.
    - ERROR | file patterns: The `vendored_frameworks` pattern did not match any file.
    - ERROR | header_mappings_dir: The header_mappings_dir (`destroot/include`) is not a directory.
    - WARN  | license: Unable to find a license file
... (NOTE) ...
[!] The spec did not pass validation, due to 4 errors and 1 warning.

Despite the warnings, there are another 4 errors.

@grabbou
Copy link
Contributor Author

grabbou commented Oct 13, 2020

As discussed on Discord, you may wanna use hermes-runtime-darwin instead to test it. The current package you're using was meant to be consumed by npm.

@Huxpro
Copy link
Contributor

Huxpro commented Oct 13, 2020

env hermes-artifact-url='https://github.com/facebook/hermes/releases/download/v0.7.1/hermes-runtime-darwin-v0.7.1.tar.gz' pod trunk push hermes-engine.podspec

--------------------------------------------------------------------------------
 🎉  Congrats

 🚀  hermes-engine (0.7.1) successfully published
 📅  October 13th, 00:57
 🌎  https://cocoapods.org/pods/hermes-engine
 👍  Tell your friends!
--------------------------------------------------------------------------------

Yay!! Looks good to me? Need to verify if it actually works!
I've committed my changes on podspec in case I did something stupid: 779a61e

@Huxpro Huxpro self-assigned this Nov 10, 2020
@Huxpro
Copy link
Contributor

Huxpro commented Nov 10, 2020

@grabbou @alloy Should this be closed? Or do we have some other ideas to further optimize the size?

@alloy
Copy link
Contributor

alloy commented Nov 11, 2020

I think this is good for now. Specifically we no longer install this dependency by default with new apps, people have to opt-in and then install it.

@Huxpro Huxpro closed this as completed Nov 11, 2020
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

No branches or pull requests

4 participants