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

Test / mitigate costly module recompilation: inheritance of OS version #225

Closed
jerrymarino opened this issue Mar 5, 2021 · 3 comments
Closed

Comments

@jerrymarino
Copy link
Contributor

jerrymarino commented Mar 5, 2021

Add the ability to inherit the OS version from the top level application down the tree. By default it'd be ideal to set the OS version for a single build. There are a few reasons this is useful.

  1. performance. As modules are cached with respect to OS version, I get O(N OS versions) module rebuilds. see lower
  2. deprecations. If I'm staticlaly linking some code into my app I'd like the compiler to alert me if it's using a deprecated or missing API

It seems like pt 1. might be marginal, however, when scaling to thousands of includes and many OS versions it is costly. Consider the build

swift_lib a - deps [b], os version 10
swift_lib b - deps [c], os version 9
swift_lib c - deps [d], os version 8
swift_lib d -deps [e] , os version 7
swift_lib e, os version 6

At each step of the build we need to reparse and recompile modules for all transitive imports because the modules for the previous step aren't reused. For specific use cases this is a non trivial cost, depending on how expensive it is to compile a PCM

@jerrymarino jerrymarino changed the title Mitigate/test costly module recompilation: inheritance of OS version Test / mitigate costly module recompilation: inheritance of OS version Mar 5, 2021
@jerrymarino
Copy link
Contributor Author

This issue is related to bazelbuild/rules_swift#575 in a way - but solves it from another, and more ideal angle: pinning the OS version downwards, and minimizing the OS version in the repo to the shipping products to reduce variants.

@jerrymarino
Copy link
Contributor Author

In my preliminary benchmark run, this cut ~5-6 mins off clean build time from the baseline, but the larger impact seems to be on specific on incremental build use cases that hit module recompilation at every library along the way.

jerrymarino added a commit that referenced this issue Jun 11, 2021
This fixes module re-use across top level modules by setting a build
level OS version for each platform.

Relates to #225.
@jerrymarino
Copy link
Contributor Author

Here is a t-shirt sized summary of the impact:

  • Changing a file in the top level module with an empty module cache: 435 seconds
  • Changing a file with a hot module cache: 50 seconds.

The same issue occurs with objc.

This is most severe in an incremental build which changes a lot modules. Generally while the build progresses we populate 95% of code into iOS 11 module cache. When it gets to the top level library ( has iOS 12 as a version ) it has to re-build the module cache for the entire tree. I'm going to work on a proposal to get in version pinning in.

Consider #258 a WIP for this, but we need to address libraries that don't go through that transition right now.

jerrymarino added a commit that referenced this issue Jun 29, 2021
It adds the ability to set `minimum_deployment_os_version` and pulls in
the latest sha of `rules_apple`.

It stopgaps target specific evaluation xcconfig issues. When I tried to
build an app with target specifc xcconfigs with settings like
`SWIFT_PLATFORM_TARGET_PREFIX`, I end up getting invalid `-target` flags
in compiler invocations which were incomplete. `rules_ios`'s xcconfig
evaluator is not a full implementation of xcconfig and conflicts with
Bazel's settings here. It doesn't currenlty evaluate on a per archicture
basis but aligning the 2 may be a longer term exercise.

This PR is related to #225 but doesn't fix it. Longer term, we'll need a
way to either remove `platforms` or find a way to pin override them in a
way like #258 takes care of.
jerrymarino added a commit that referenced this issue Jun 30, 2021
* minimum_deployment_os_version / `-target` stopgap

It adds the ability to set `minimum_deployment_os_version` and pulls in
the latest sha of `rules_apple`.

It stopgaps target specific evaluation xcconfig issues. When I tried to
build an app with target specifc xcconfigs with settings like
`SWIFT_PLATFORM_TARGET_PREFIX`, I end up getting invalid `-target` flags
in compiler invocations which were incomplete. `rules_ios`'s xcconfig
evaluator is not a full implementation of xcconfig and conflicts with
Bazel's settings here. It doesn't currenlty evaluate on a per archicture
basis but aligning the 2 may be a longer term exercise.

This PR is related to #225 but doesn't fix it. Longer term, we'll need a
way to either remove `platforms` or find a way to pin override them in a
way like #258 takes care of.
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

2 participants