net/nimble/host code size reduction

[Christopher Collins <cc...@apache.org>]

Hello all,

Yesterday I made a fairly large commit to the net/nimble/host package in the
core repository (the host-side of the BLE stack).  I wanted to provide the
community with an explanation of these changes and related changes planned for
the future.  This email will probably be long and detailed, so don't
feel bad about skimming it for anything that you find interesting.  That
said, I would love to hear any and all feedback.

First, here is the relevant commit:
    Repo: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core
    Branch: develop
    Commit: 2fef21d6a1f9d3d83b88ecf11b825a6e5a354a47

Motivation for this change: the code size of net/nimble/host had grown
quite large over the last few months.  During development of the host,
most focus was put on correctness and maintainability, while speed and
size considerations were placed in the background.  Now that the host is
reasonably complete, it is a good time to start optimizing, with the
hope that the unit tests will protect against any regressions.

The commit consists of two major changes:

    1. Use packed structs for over-the-air ATT and L2CAP commands.
    2. Compile-out most assertions unless BLE_HS_DEBUG is defined.

Regarding 1:

Prior to this change, the host code was manually
marshalling fields between buffers and properly-aligned structs.  This
had the benefit of being defined behavior according to the C standard,
but at the cost of increased code size.  By using the gcc __packed__
attribute, the compiler is able to take advantage of the Cortex-M's
ability to perform unaligned accesses, resulting in a substantial code
savings.

Regarding 2:

The host code makes liberal use of the assert() macro.
Each invocation of the assert() macro was adding a surprising amount of
code to the resulting binary.  The size increase is not caused by
strings being embedded in the binary; the baselibc assert() does not include
the expression text in its expansion, and gcc collapses duplicate
filename strings.  The size increase is just caused by the comparison
and branch implicit in the assert macro.

The asserts in the host code are plentiful and conservative.  They
caused an unwelcome increase in code size, but I didn't want to remove
them entirely because they are valuable during regression testing.  The
standard assert() macro compiles to nothing if the NDEBUG symbol is
defined, but I wanted more control over which asserts actually get
disabled.  As a compromise, I added a second type of assertion that only
gets compiled in if the BLE_HS_DEBUG symbol is defined.  

I think the concept of multiple assert levels will be useful to other
packages and that it should be added to the core OS.  For this reason, I
intentionally made the new assert macro names ugly, as I hope to replace
them with something that isn't host-specific.

Here are the two macros I added:

#if BLE_HS_DEBUG
    #define BLE_HS_DBG_ASSERT(x) assert(x)
    #define BLE_HS_DBG_ASSERT_EVAL(x) assert(x)
#else
    #define BLE_HS_DBG_ASSERT(x)
    #define BLE_HS_DBG_ASSERT_EVAL(x) ((void)(x))
#endif


BLE_HS_DBG_ASSERT(x) is self-explanatory; it asserts if the necessary
debug symbol is defined.

BLE_HS_DBG_ASSERT_EVAL(x) is less obvious.  If the BLE_HS_DEBUG symbol
is defined, this macro has the same effect as the first one.  If the
symbol is not defined, this macro evaluates its argument and throws away
the result.  The reason this macro exists is to prevent gcc from raising
"variable set but not used" warnings in code like the following:

    rc = operation_that_should_never_fail();
    BLE_HS_DBG_ASSERT_EVAL(rc == SUCCESS);

Note that the BLE_HS_DBG_ASSERT_EVAL() macro differs from the standard
assert() macro in this respect.  The standard assert() macro does not
evalute its argument if NDEBUG is defined, which allows you to write
code like the following:

    assert(expensive_sanity_check() == SUCCESS);

Future changes:

There are a number of future changes planned for further reducing the
host code size.

    * Pack the HCI command and event structs.
    * Rework (or remove / replace) the generic FSM implementation; the
      code has outgrown this facility.

If you have made this far and have any suggestions of your own, they
would be much appreciated, so please feel free to share them.

Thanks,
Chris

Re: net/nimble/host code size reduction

[Christopher Collins <cc...@apache.org>]

On Tue, Apr 05, 2016 at 03:10:40PM -0700, aditi hilbert wrote:
> So how much was the size reduction?

Good question :).  The host size was reduced

    from: 48526 bytes
      to: 42257 bytes

I should note that I am building with -DLOG_LEVEL=2, which also reduces
the code size somewhat.  This option sets the log level to info rather
than the default (debug).

Chris

Re: net/nimble/host code size reduction

[aditi hilbert <ad...@runtime.io>]

So how much was the size reduction?

thanks,
aditi

> On Apr 5, 2016, at 12:41 PM, Christopher Collins <cc...@apache.org> wrote:
> 
> Hello all,
> 
> Yesterday I made a fairly large commit to the net/nimble/host package in the
> core repository (the host-side of the BLE stack).  I wanted to provide the
> community with an explanation of these changes and related changes planned for
> the future.  This email will probably be long and detailed, so don't
> feel bad about skimming it for anything that you find interesting.  That
> said, I would love to hear any and all feedback.
> 
> First, here is the relevant commit:
>    Repo: http://git-wip-us.apache.org/repos/asf/incubator-mynewt-core
>    Branch: develop
>    Commit: 2fef21d6a1f9d3d83b88ecf11b825a6e5a354a47
> 
> Motivation for this change: the code size of net/nimble/host had grown
> quite large over the last few months.  During development of the host,
> most focus was put on correctness and maintainability, while speed and
> size considerations were placed in the background.  Now that the host is
> reasonably complete, it is a good time to start optimizing, with the
> hope that the unit tests will protect against any regressions.
> 
> The commit consists of two major changes:
> 
>    1. Use packed structs for over-the-air ATT and L2CAP commands.
>    2. Compile-out most assertions unless BLE_HS_DEBUG is defined.
> 
> Regarding 1:
> 
> Prior to this change, the host code was manually
> marshalling fields between buffers and properly-aligned structs.  This
> had the benefit of being defined behavior according to the C standard,
> but at the cost of increased code size.  By using the gcc __packed__
> attribute, the compiler is able to take advantage of the Cortex-M's
> ability to perform unaligned accesses, resulting in a substantial code
> savings.
> 
> Regarding 2:
> 
> The host code makes liberal use of the assert() macro.
> Each invocation of the assert() macro was adding a surprising amount of
> code to the resulting binary.  The size increase is not caused by
> strings being embedded in the binary; the baselibc assert() does not include
> the expression text in its expansion, and gcc collapses duplicate
> filename strings.  The size increase is just caused by the comparison
> and branch implicit in the assert macro.
> 
> The asserts in the host code are plentiful and conservative.  They
> caused an unwelcome increase in code size, but I didn't want to remove
> them entirely because they are valuable during regression testing.  The
> standard assert() macro compiles to nothing if the NDEBUG symbol is
> defined, but I wanted more control over which asserts actually get
> disabled.  As a compromise, I added a second type of assertion that only
> gets compiled in if the BLE_HS_DEBUG symbol is defined.  
> 
> I think the concept of multiple assert levels will be useful to other
> packages and that it should be added to the core OS.  For this reason, I
> intentionally made the new assert macro names ugly, as I hope to replace
> them with something that isn't host-specific.
> 
> Here are the two macros I added:
> 
> #if BLE_HS_DEBUG
>    #define BLE_HS_DBG_ASSERT(x) assert(x)
>    #define BLE_HS_DBG_ASSERT_EVAL(x) assert(x)
> #else
>    #define BLE_HS_DBG_ASSERT(x)
>    #define BLE_HS_DBG_ASSERT_EVAL(x) ((void)(x))
> #endif
> 
> 
> BLE_HS_DBG_ASSERT(x) is self-explanatory; it asserts if the necessary
> debug symbol is defined.
> 
> BLE_HS_DBG_ASSERT_EVAL(x) is less obvious.  If the BLE_HS_DEBUG symbol
> is defined, this macro has the same effect as the first one.  If the
> symbol is not defined, this macro evaluates its argument and throws away
> the result.  The reason this macro exists is to prevent gcc from raising
> "variable set but not used" warnings in code like the following:
> 
>    rc = operation_that_should_never_fail();
>    BLE_HS_DBG_ASSERT_EVAL(rc == SUCCESS);
> 
> Note that the BLE_HS_DBG_ASSERT_EVAL() macro differs from the standard
> assert() macro in this respect.  The standard assert() macro does not
> evalute its argument if NDEBUG is defined, which allows you to write
> code like the following:
> 
>    assert(expensive_sanity_check() == SUCCESS);
> 
> Future changes:
> 
> There are a number of future changes planned for further reducing the
> host code size.
> 
>    * Pack the HCI command and event structs.
>    * Rework (or remove / replace) the generic FSM implementation; the
>      code has outgrown this facility.
> 
> If you have made this far and have any suggestions of your own, they
> would be much appreciated, so please feel free to share them.
> 
> Thanks,
> Chris