mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

[ios] Update generated Xcode project with -Os & -flto #16460

Closed julianrex closed 4 years ago

julianrex commented 4 years ago

See https://github.com/mapbox/mapbox-gl-native-ios/issues/134 for context.

This PR adds -Os and -flto for the Release and RelWithDebInfo configurations, both of which are currently used by the iOS Maps SDK release process. (This is a possible first step in transitioning iOS to just use Release & Debug configurations).

It turned out it wasn't sufficient to set these optimization settings by "just" changing the gcc flags, instead it needed to be changed with:

set_target_properties(${target} PROPERTIES XCODE_ATTRIBUTE_GCC_OPTIMIZATION_LEVEL[variant=Release] $<$<CONFIG:RELEASE>:s>)

There may be a better way (I'm definitely a cmake novice), so would appreciate any improvements.

cc @mapbox/maps-ios @1ec5

julianrex commented 4 years ago

As expected, this change has affected performance (both negative and positive), but the generated framework size is much smaller (149Mb -> 72Mb).

@alexshalamov what's your reading of the performance changes?

/cc @knov

knov commented 4 years ago

thoughts here @alexshalamov @1ec5

/cc @chloekraw

alexshalamov commented 4 years ago

@julianrex Have you tried running benchmarks and checking difference between -Os an -Oz? If not and it is not super-urgent to land this PR, I can try running benchmarks on device and see if there is a significant difference.

julianrex commented 4 years ago

I haven't tried checking the performance for -Oz, though the binary size improvement was significant.

Given the SDK was previously built with -Os I didn't want to jump straight to -Oz 😁

An alternative to this PR might be to have cmake specify an xcconfig file that optionally includes a platform xcconfig file. This would allow the iOS SDKs to override any of the build settings. However, I'm a little uncomfortable with that.

I do think there is scope in gl-native to fine tune the optimizations on a per-file basis. It's a lot of effort, but could give us a good balance of size vs speed.

1ec5 commented 4 years ago

Remember to say something about this build configuration change in the changelog.

1ec5 commented 4 years ago

(Oh also, if you can make these same changes to macos.cmake, I’d be much obliged.)

alexshalamov commented 4 years ago

@julianrex didn't see much difference in performance benchmarks between -Os and -Oz -Os https://circleci.com/gh/mapbox/mapbox-gl-native/436724#artifacts/containers/0 -Oz https://circleci.com/gh/mapbox/mapbox-gl-native/436745#artifacts/containers/0

julianrex commented 4 years ago

~@alexshalamov Once I update macOS, how do I change the benchmark baselines so the benchmark runners don't fail?~

How did the iOS benchmark runner go green below??

alexshalamov commented 4 years ago

@julianrex @knov -Os regresses performance for iOS targets, for almost all tests. In some cases > 15% performance drop. results_comparison.txt

Maybe worth re-thinking some of the flags or making two builds? One optimized for performance while the other for binary size?

1ec5 commented 4 years ago

For context, we switched from -O3 to -Os in #4641 on iOS and OS X and in #8253 for all platforms. At the time, that change nearly halved the size of the framework binary with no perceivable performance hit.

When we (inadvertently) switched back to -O3, how much of a binary size regression did we see?

stale[bot] commented 4 years ago

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.