Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Update compile flags for iOS #7131

Closed
wants to merge 5 commits into from
Closed

Update compile flags for iOS #7131

wants to merge 5 commits into from

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Nov 21, 2016

We should do these changes to iOS compilation settings:

/cc @1ec5

@1ec5
Copy link
Contributor

1ec5 commented Nov 21, 2016

Only build active architecture when building Debug builds to speed up compile time

This means we won't catch any differences between 32-bit and 64-bit. We have unit tests for regressions around the size of an NSInteger, for example.

@friedbunny
Copy link
Contributor

Strip non-global symbols for Release static/dynamic library

We already conditionally do this in the packaging script (just not in the Xcode project).

@mention-bot
Copy link

@kkaefer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @boundsj and @incanus to be potential reviewers.

@kkaefer
Copy link
Member Author

kkaefer commented Nov 21, 2016

This means we won't catch any differences between 32-bit and 64-bit. We have unit tests for regressions around the size of an NSInteger, for example.

Okay; can we force ONLY_ACTIVE_ARCH=NO on CI, but use YES (first commit in this PR) for regular builds that you do when developing?

@kkaefer
Copy link
Member Author

kkaefer commented Nov 21, 2016

We already conditionally do this in the packaging script (just not in the Xcode project)

Any reason why we wouldn't do this as part of the Xcode build and only do it in an external packaging script?

@kkaefer
Copy link
Member Author

kkaefer commented Nov 21, 2016

With all of the above optimizations, arm64 shrinks from 3,351 KB to 2,925 KB.

@1ec5
Copy link
Contributor

1ec5 commented Nov 21, 2016

I’m all for performing the stripping in the Xcode project instead of the packaging script. The more of the packaging script we can put in proper Xcode build steps, the better.

This means we won't catch any differences between 32-bit and 64-bit. We have unit tests for regressions around the size of an NSInteger, for example.

Okay; can we force ONLY_ACTIVE_ARCH=NO on CI, but use YES (first commit in this PR) for regular builds that you do when developing?

I’d prefer not to trim the CI build matrix at the expense of local incremental build times. We already make this compromise when it comes to static builds, which is why #6194 went undetected for awhile.

@kkaefer
Copy link
Member Author

kkaefer commented Nov 22, 2016

I’d prefer not to trim the CI build matrix at the expense of local incremental build times.

It's the other way around: Local build times would be faster, since we are only building the active architecture for Debug builds (typically arm64 with most modern devices). On CI, we'd explicitly set ONLY_ACTIVE_ARCH=NO on the Debug build to have it build both architectures. Release builds will still build both architectures local and on CI.

@kkaefer kkaefer force-pushed the 7131-ios-compile-flags branch from 91cb49d to 6e68b89 Compare November 22, 2016 15:49
@friedbunny
Copy link
Contributor

Any reason why we wouldn't do this as part of the Xcode build and only do it in an external packaging script?

We can do it in the Xcode project, but the implementation of conditionally stripped symbols will have to be updated in the packaging script and all of the possible packages will need to be re-validated.

@kkaefer
Copy link
Member Author

kkaefer commented Nov 22, 2016

all of the possible packages will need to be re-validated.

What does that mean? Are you referring to code signing?

@friedbunny
Copy link
Contributor

I mean that we’ll need to confirm that every built package still works as expected.

@kkaefer kkaefer force-pushed the 7131-ios-compile-flags branch from 6e68b89 to 68b3723 Compare November 22, 2016 20:18
@kkaefer
Copy link
Member Author

kkaefer commented Nov 22, 2016

@friedbunny what's the process here? Do we have a list of all the packages that need to build, and how to validate them?

@friedbunny
Copy link
Contributor

friedbunny commented Nov 22, 2016

Here’s the list of packages we publish — static and dynamic w/symbols, static and dynamic w/o symbols, and one Fabric build that’s static w/o symbols.

The process for verifying that a build is usable is mostly manual. You’ll need at least two test iOS projects that integrate Mapbox.framework — one for static and one for dynamic.

Steps to confirm that a build works:

  • Test it in an iOS project.
    • With and without storyboards.
    • ObjC and Swift.
  • Submit that app to TestFlight/App Store.
  • Confirm that the dSYM and framework UUIDs match. (automatic)

These steps should be repeated for every packaged build.

I’m also unsure where we stand on Xcode 8 vs 7.3.1 (#7029) — it may be necessary to test builds from both. /cc @boundsj

@kkaefer
Copy link
Member Author

kkaefer commented Nov 23, 2016

The process for verifying that a build is usable is mostly manual.

Is there a way to automate it? It seems that I can't run deploy-packages.sh when that version has already been published.

What is the advantage of building with symbols? I thought we have external symbols in the .dSYM file anyway?

You’ll need at least two test iOS projects that integrate Mapbox.framework — one for static and one for dynamic.

Where do I get those from? Just create a new sample app that embeds those frameworks? Can we automate this?

@friedbunny
Copy link
Contributor

friedbunny commented Jul 20, 2017

I looked at implementing LTO today and wasn’t able to observe any change in the bench app FPS on my iPhone 5 — plain master and various LTO combinations all averaged ~40.5 FPS. After watching the WWDC sessions that introduced LTO and ThinLTO, this was underwhelming.

Notes

  • ThinLTO seems like the way to go (rather than Monolithic) — compile times are supposed to be much better, while performance is supposed to be more or less the same.
  • LTO appeared compatible with ccache (surprisingly).
  • Problematic: missing core symbol warnings from dsymutil when building the dynamic framework.
  • Build times didn’t seem much worse, especially with ccache.
  • A more comprehensive benchmark may reveal performance changes.

Links

@kkaefer
Copy link
Member Author

kkaefer commented Jul 20, 2017

LTO optimizes the CPU code, but it looks like rendering is GPU bound on your phone. This would explain the lack of FPS increase. Did LTO have any effect on binary size?

@friedbunny
Copy link
Contributor

#12502 takes on LTO, the final unmerged piece of this PR — closing.

@friedbunny friedbunny closed this Jul 27, 2018
@friedbunny friedbunny deleted the 7131-ios-compile-flags branch July 27, 2018 23:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants