#9021: Fixed static framework size by RomainQuidet · Pull Request #9022 · mapbox/mapbox-gl-native

RomainQuidet · 2017-05-17T15:24:15Z

Fix #9021 by using correct llvm flags on release scheme for strip command.

1ec5

This looks OK to me, but I’d like to get more 👀 on it from folks who’ve worked on our packaging more recently. Not sure whether our -symbols packaging would override this setting.

1ec5 · 2017-05-17T21:58:32Z

If this change should go into iOS SDK v3.6.0, this PR would need to be retargeted and rebased onto the release-ios-v3.6.0-android-v5.1.0 branch. Otherwise, this note should be moved up to the “master” section, which will go into iOS SDK v3.7.0.

/cc @boundsj

friedbunny · 2017-05-18T16:58:30Z

Not sure whether our -symbols packaging would override this setting.

Indeed, I don’t see how setting GCC_GENERATE_DEBUGGING_SYMBOLS=NO for the Release scheme would create proper -symbols builds — the make ipackage command relies on the symbols always being generated and optionally strips them with make ipackage-strip or SYMBOLS=NO.

And yet, this does appear to work. The number of symbols is roughly the same as in v3.6.0-alpha.1:

$ make ipackage BUILDTYPE=Release FORMAT=static
...
$ nm static/Mapbox.framework/Mapbox | grep MGL | wc -l
4563
$ strip -Sx static/Mapbox.framework/Mapbox-stripped
$ nm static/Mapbox.framework/Mapbox-stripped | grep MGL | wc -l
1910

$ nm mapbox-ios-sdk-3.6.0-alpha.1-symbols/static/Mapbox.framework/Mapbox | grep MGL | wc -l
4553
$ nm mapbox-ios-sdk-3.6.0-alpha.1/static/Mapbox.framework/Mapbox | grep MGL | wc -l
1910

And file sizes are decreased:

$ du -h static/Mapbox.framework/Mapbox*
191M	static/Mapbox.framework/Mapbox
154M	static/Mapbox.framework/Mapbox-stripped

461M	mapbox-ios-sdk-3.6.0-alpha.1-symbols/static/Mapbox.framework/Mapbox
256M	mapbox-ios-sdk-3.6.0-alpha.1/static/Mapbox.framework/Mapbox

Until we understand the mechanism at work here, I wouldn’t feel comfortable merging this PR.

friedbunny

Let’s hold this until it can be fully vetted and the mechanism explained.

RomainQuidet · 2017-05-23T14:44:22Z

Documentation about this:

https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/SymbolVisibility.html

…ase scheme for strip command

boundsj · 2017-06-01T18:16:34Z

@friedbunny I re-targeted this at the release branch for v3.6.0 and convinced the bots to run the tests. What are you thinking the next steps we need to do are before we can land this?

friedbunny · 2017-06-01T19:29:34Z

Explain why GCC_GENERATE_DEBUGGING_SYMBOLS = NO generates debug symbols.
Ensure that dSYMs still work.
Ensure that both framework flavors still work.

boundsj · 2017-06-02T05:21:55Z

@friedbunny here is an update:

Despite what was suggested in the SO post mentioned in #9021 (comment), GCC_SYMBOLS_PRIVATE_EXTERN does not affect the size of our binaries. I've been testing with only the GCC_GENERATE_DEBUGGING_SYMBOLS portion of this proposed change which does lead to the size reductions noted here and in #9021

Explain why GCC_GENERATE_DEBUGGING_SYMBOLS = NO generates debug symbols.

I think this has to do with the ability to attach and use a debugger to step through implementations in the binary. When it is set to NO, the resulting static binary shrinks significantly and is not visible in the debugger. When it is set to YES, the resulting binary is significantly larger, is visible in the debugger, and these debugging symbols cannot be fully stripped by our current strip usage in the package script. Assuming a dSYM for the host application with the symbols can still be created (I think it can, see below) then I think it actually makes sense to set this flag to NO for a static release build.

Ensure that dSYMs still work.

I created an app using the static Mapbox Map SDK I created with GCC_GENERATE_DEBUGGING_SYMBOLS = NO and make ipackage FORMAT=static BUILDTYPE=Release SYMBOLS=YES and that app's dSYM appears to have all of our map SDK symbols. Of course, if I set a breakpoint on a call to one of our map SDK APIs and try to step in, I cannot.

Ensure that both framework flavors still work.

I think you mean that we should test the dynamic framework, too. This proposal changes only the static target when built for release so far. I think we definitely do not want to make this change for the dynamic framework that produces a standalone dSYM file when built for release.

So, I think GCC_GENERATE_DEBUGGING_SYMBOLS = NO for static release builds makes sense. I'd still like to do a roundtrip test with a simulated crash in an app that uses this sort of build though. Also, I think that the GCC_SYMBOLS_PRIVATE_EXTERN portion of this change should probably be reverted.

friedbunny · 2017-06-02T19:08:05Z

This proposal changes only the static target when built for release so far.

It looks like the changes affect all of mbgl_platform_core (in cmake) and any Release build (in the Xcode project directly) — is that not the case?

friedbunny · 2017-06-14T06:23:15Z

The dynamic framework does change, but in apparently harmless/positive ways:

Mapbox.framework/Mapbox size decreases from 64MB to 59MB.
Bitcode-less ARM64 slice size decreases from 4.0MB to 3.6MB.
dSYM size decreases from 87MB to 84MB.

Here’s a diff of nm dynamic/Mapbox.framework/Mapbox | grep -i m*gl — it shows a decrease from ~3130 symbols to ~350 symbols. The removed symbols are almost entirely from core mbgl. Does this follow your expectation, @kkaefer?

After running both the static and dynamic frameworks in apps, I don’t see any compile or runtime issues.

friedbunny

@fabian-guerra @boundsj Let’s give this a shot in b3, then back it out if any issues arise.

friedbunny · 2017-06-14T06:50:06Z

I updated the changelog entry — we should also reword the commit message when this is merged.

kkaefer · 2017-06-14T09:46:23Z

There's a similar PR in #7604 that achieves the same thing by removing symbol visibility. We completed it for macOS, but it stalled for iOS. When removing symbols, we have to make sure we're not accidentally stripping symbol visibility for things that are declared in the public headers but not used anywhere.

kkaefer · 2017-06-14T09:49:29Z

In #7604, we're using -fvisibility=hidden, which is what GCC_SYMBOLS_PRIVATE_EXTERN ultimately does as well. However, I've found that Xcode didn't add it for dynamic libraries (which makes sense because it can't know what symbols are used), which is why we're using the flag directly and manually mark up exported symbols instead.

kkaefer · 2017-06-14T09:51:40Z

Note that there is a script called platform/darwin/scripts/check-public-symbols.js which allows you to check that all public symbols (things exposed in public headers) have an export keyword. Our current packaging workflow only does this for macOS, but it can be changed to report missing symbols for both.

friedbunny · 2017-06-14T18:11:09Z

In that case, let’s hold here and revisit/reevaluate #7604. If we don’t come to a determination today, I think this is the type of fix we should make a patch release for.

friedbunny · 2017-07-10T18:08:40Z

The symbol visibility portion of this PR (GCC_SYMBOLS_PRIVATE_EXTERN) is being addressed by #7604, but that has no effect on the size of the static framework.

That means that we need to refocus this PR on GCC_GENERATE_DEBUGGING_SYMBOLS and see if that alone achieves a reduction in the size of the static framework.

friedbunny · 2017-07-17T21:50:02Z

These debug symbols are useful for development and are stripped in the archive/App Store submission process, so they don’t contribute to the effective download size of your app. To that end, I think we should keep GCC_GENERATE_DEBUGGING_SYMBOLS=yes for the static library and close this PR.

As an open source project, we want to enable developers to see the internal workings of our SDK and be able to easily open issues, file bug reports, and (with any luck) make contributions. If it’s not possible to step-debug our SDK in the context of a developer’s own app, that throws up a barrier to contribution.

It definitely is true that these debug symbols add significantly to the size of the uncompressed static binary (about 60%) and we should keep investigating less invasive ways to bring this size down.

Binary size metrics.

make ipackage BUILDTYPE=Release SYMBOLS=NO FORMAT=static
Xcode 8.3.3, unless otherwise noted

master @ `8ae7010`

* Measured 20,782,340 bytes for 'Platform=iOS,Arch=armv7' (build/ios/pkg/static/Mapbox-stripped-armv7)
* Measured 23,613,712 bytes for 'Platform=iOS,Arch=arm64' (build/ios/pkg/static/Mapbox-stripped-arm64)
* Measured 23,416,356 bytes for 'Platform=iOS,Arch=i386' (build/ios/pkg/static/Mapbox-stripped-i386)
* Measured 25,703,792 bytes for 'Platform=iOS,Arch=x86_64' (build/ios/pkg/static/Mapbox-stripped-x86_64)
* Measured 93,516,168 bytes for 'Platform=iOS,Arch=static' (build/ios/pkg/static/Mapbox-stripped)
= 280.8 MB total

Universal app: 7 MB compressed, 21.1 MB uncompressed
iPhone app: 3.4 MB compressed, 10.6 MB uncompressed

GCC_GENERATE_DEBUGGING_SYMBOLS=NO @ `8ae7010`

* Measured 20,782,260 bytes for 'Platform=iOS,Arch=armv7' (build/ios/pkg/static/Mapbox-stripped-armv7)
* Measured 22,656,736 bytes for 'Platform=iOS,Arch=arm64' (build/ios/pkg/static/Mapbox-stripped-arm64)
* Measured 22,892,116 bytes for 'Platform=iOS,Arch=i386' (build/ios/pkg/static/Mapbox-stripped-i386)
* Measured 25,702,352 bytes for 'Platform=iOS,Arch=x86_64' (build/ios/pkg/static/Mapbox-stripped-x86_64)
* Measured 92,033,432 bytes for 'Platform=iOS,Arch=static' (build/ios/pkg/static/Mapbox-stripped)
= 171 MB total

Universal app: 7 MB compressed, 21.1 MB uncompressed
iPhone app: 3.4 MB compressed, 10.6 MB uncompressed

MACH_O_TYPE = mh_object; DEAD_CODE_STRIPPING = NO; @ `8ae7010`

* Measured  7,140,684 bytes for 'Platform=iOS,Arch=armv7' (build/ios/pkg/static/Mapbox-stripped-armv7)
* Measured  8,659,480 bytes for 'Platform=iOS,Arch=arm64' (build/ios/pkg/static/Mapbox-stripped-arm64)
* Measured  8,702,508 bytes for 'Platform=iOS,Arch=i386' (build/ios/pkg/static/Mapbox-stripped-i386)
* Measured  9,698,768 bytes for 'Platform=iOS,Arch=x86_64' (build/ios/pkg/static/Mapbox-stripped-x86_64)
* Measured 34,201,408 bytes for 'Platform=iOS,Arch=static' (build/ios/pkg/static/Mapbox-stripped)
= 109.9 MB total

Universal app: 7 MB compressed, 21.1 MB uncompressed
iPhone app: 3.4 MB compressed, 10.6 MB uncompressed

master @ `8ae7010` with Xcode 8.2.1

* Measured 20,972,876 bytes for 'Platform=iOS,Arch=armv7' (build/ios/pkg/static/Mapbox-stripped-armv7)
* Measured 22,885,392 bytes for 'Platform=iOS,Arch=arm64' (build/ios/pkg/static/Mapbox-stripped-arm64)
* Measured 23,624,116 bytes for 'Platform=iOS,Arch=i386' (build/ios/pkg/static/Mapbox-stripped-i386)
* Measured 26,042,336 bytes for 'Platform=iOS,Arch=x86_64' (build/ios/pkg/static/Mapbox-stripped-x86_64)
* Measured 93,524,688 bytes for 'Platform=iOS,Arch=static' (build/ios/pkg/static/Mapbox-stripped)
= 281.3 MB total

master @ `8ae7010` with Xcode 7.3.1

* Measured 21,116,988 bytes for 'Platform=iOS,Arch=armv7' (build/ios/pkg/static/Mapbox-stripped-armv7)
* Measured 23,044,544 bytes for 'Platform=iOS,Arch=arm64' (build/ios/pkg/static/Mapbox-stripped-arm64)
* Measured 23,782,908 bytes for 'Platform=iOS,Arch=i386' (build/ios/pkg/static/Mapbox-stripped-i386)
* Measured 26,173,912 bytes for 'Platform=iOS,Arch=x86_64' (build/ios/pkg/static/Mapbox-stripped-x86_64)
* Measured 94,118,320 bytes for 'Platform=iOS,Arch=static' (build/ios/pkg/static/Mapbox-stripped)
= 307 MB total

friedbunny · 2017-07-17T22:31:13Z

For comparison, the size of realm-cocoa’s uncompressed static library is 602 MB.

RomainQuidet mentioned this pull request May 17, 2017

[iOS] Static lib size x2 from 3.4.2 to 3.5.2 #9021

Closed

RomainQuidet changed the title ~~9021: Fixed static framework size~~ #9021: Fixed static framework size May 17, 2017

jfirebaugh requested a review from 1ec5 May 17, 2017 16:34

boundsj added build iOS Mapbox Maps SDK for iOS labels May 17, 2017

boundsj added this to the ios-v3.6.0 milestone May 17, 2017

1ec5 approved these changes May 17, 2017

View reviewed changes

1ec5 requested a review from friedbunny May 17, 2017 22:01

friedbunny suggested changes May 18, 2017

View reviewed changes

9021: Fixed static framework size by using correct llvm flags on rele…

09459b3

…ase scheme for strip command

boundsj force-pushed the 9021 branch from f0b691a to 09459b3 Compare June 1, 2017 18:12

boundsj changed the base branch from master to release-ios-v3.6.0-android-v5.1.0 June 1, 2017 18:14

friedbunny approved these changes Jun 14, 2017

View reviewed changes

Reword changelog entry and move it to packaging section

39eda94

friedbunny added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 14, 2017

boundsj modified the milestones: ios-v3.7.0, ios-v3.6.0 Jun 22, 2017

friedbunny mentioned this pull request Jul 7, 2017

Add -fvisibility=hidden to iOS #7604

Merged

friedbunny mentioned this pull request Jul 17, 2017

[build] Compile iOS static library as Relocatable Object File #9530

Closed

friedbunny closed this Jul 18, 2017

Conversation

RomainQuidet commented May 17, 2017 • edited by friedbunny Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

1ec5 May 17, 2017

Choose a reason for hiding this comment

Uh oh!

friedbunny commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

friedbunny left a comment

Choose a reason for hiding this comment

Uh oh!

RomainQuidet commented May 23, 2017

Uh oh!

boundsj commented Jun 1, 2017

Uh oh!

friedbunny commented Jun 1, 2017

Uh oh!

boundsj commented Jun 2, 2017

Uh oh!

friedbunny commented Jun 2, 2017

Uh oh!

friedbunny commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

friedbunny left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friedbunny commented Jun 14, 2017

Uh oh!

kkaefer commented Jun 14, 2017

Uh oh!

kkaefer commented Jun 14, 2017

Uh oh!

kkaefer commented Jun 14, 2017

Uh oh!

friedbunny commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

friedbunny commented Jul 10, 2017

Uh oh!

friedbunny commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

master @ 8ae7010

GCC_GENERATE_DEBUGGING_SYMBOLS=NO @ 8ae7010

MACH_O_TYPE = mh_object; DEAD_CODE_STRIPPING = NO; @ 8ae7010

master @ 8ae7010 with Xcode 8.2.1

master @ 8ae7010 with Xcode 7.3.1

Uh oh!

friedbunny commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RomainQuidet commented May 17, 2017 •

edited by friedbunny

Loading

friedbunny commented May 18, 2017 •

edited

Loading

friedbunny commented Jun 14, 2017 •

edited

Loading

friedbunny left a comment •

edited

Loading

friedbunny commented Jun 14, 2017 •

edited

Loading

friedbunny commented Jul 17, 2017 •

edited

Loading

master @ `8ae7010`

GCC_GENERATE_DEBUGGING_SYMBOLS=NO @ `8ae7010`

MACH_O_TYPE = mh_object; DEAD_CODE_STRIPPING = NO; @ `8ae7010`

master @ `8ae7010` with Xcode 8.2.1

master @ `8ae7010` with Xcode 7.3.1