mapbox/variant

Remove VARIANT_INLINE

Opened this issue · 4 comments

jfirebaugh commented

It appears that some inadvertent changes have disabled the effect of the VARIANT_INLINE macro in debug builds on Windows and release builds on all other platforms, by commenting out the replacement text of the macro.

variant/include/mapbox/variant.hpp

Line 39 in 256ddd5

# define VARIANT_INLINE //__declspec(noinline)

variant/include/mapbox/variant.hpp

Line 43 in 256ddd5

# define VARIANT_INLINE //inline __attribute__((always_inline))

In those cases, the methods tagged with VARIANT_INLINE were inlined or not according to the default behavior of the compiler and optimization settings.

In my tests with mapbox-gl-native, removing the comments and thus restoring VARIANT_INLINE to its intended function in the release build causes a 2.5% size increase in the resulting binary (Apple clang 9, building with -Os):

     VM SIZE                                 FILE SIZE
 --------------                           --------------
  +3.9%  +116Ki __TEXT,__text              +116Ki  +3.9%
  +283% +2.50Ki Table of Non-instructions +2.50Ki  +283%
  +2.2% +1.36Ki Code Signature            +1.36Ki  +2.2%
   +11%    +368 [__LINKEDIT]                    0  [ = ]
  +0.0%     +48 __TEXT,__const                +48  +0.0%
  -1.2%    -224 Function Start Addresses     -224  -1.2%
  -2.2% -1.66Ki __TEXT,__unwind_info      -1.66Ki  -2.2%
 -56.1% -1.89Ki [__TEXT]                  -1.89Ki -56.7%
  -2.3% -9.11Ki __TEXT,__gcc_except_tab   -9.11Ki  -2.3%
  +2.5%  +108Ki TOTAL                      +107Ki  +2.5%

I suggest we remove the VARIANT_INLINE macro entirely, the rationale being:

  • The compiler is best positioned to determine whether or not to inline these methods, based on program analysis and the developer's chosen optimization settings.
  • VARIANT_INLINE has effectively been a no-op in release builds on the majority of platforms since 372d7c8, and we haven't noticed any issues with that.
  • Restoring it to force inlining in release builds seems to increase the binary size, the opposite of the desired effect.

cc @artemp @springmeyer @lightmare

springmeyer commented

👍 on removing. Questions I have:

  • should we keep inline? I presume not since is mostly useful for avoiding odr problems and we seem to have been avoiding those without it?
  • Are there other ways to reduce code size, perhaps like moving on noexcept support? #91
jfirebaugh commented

With #define VARIANT_INLINE inline, the size increase is even larger. I don't really understand why. Maybe the methods end up mostly getting inlined but then the non-inline copy remains present as well?

     VM SIZE                                 FILE SIZE
 --------------                           --------------
  +4.5%  +134Ki __TEXT,__text              +134Ki  +4.5%
  +290% +2.56Ki Table of Non-instructions +2.56Ki  +290%
  +2.6% +1.62Ki Code Signature            +1.62Ki  +2.6%
  +2.8%     +96 [__LINKEDIT]                    0  [ = ]
  +1.7%     +32 [__DATA]                      +32  +0.9%
  +0.1%      +8 Binding Info                   +8  +0.1%
  -0.1%     -32 __DATA,__data                 -32  -0.1%
  -1.6%    -296 Function Start Addresses     -296  -1.6%
 -20.3%    -700 [__TEXT]                     -700 -20.5%
  -3.4% -2.58Ki __TEXT,__unwind_info      -2.58Ki  -3.4%
  -1.8% -6.91Ki __TEXT,__gcc_except_tab   -6.91Ki  -1.8%
  +3.0%  +128Ki TOTAL                      +127Ki  +3.0%
springmeyer commented

Interesting. And you are measuring size with https://github.com/google/bloaty? What are the command line arguments you are passing (if another person were to try to replicate with another compiler)?

jfirebaugh commented

Yeah, my methodology is:

  • BUILDTYPE=Release make xpackage
  • cp build/macos/Release/Mapbox.framework/Mapbox Mapbox-before
  • Make modifications
  • BUILDTYPE=Release make xpackage
  • bloaty build/macos/Release/Mapbox.framework/Mapbox -- Mapbox-before
  • 👍1