[Freeciv-Dev] Re: (PR#7195) inlining map_pos_to_index

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7195 >


Since inline functions are always less effective than macros, what is the
performance *loss* by going from the previous macro form to the inline one.
What is the justification for making the change from macro to inline given
all the inherent problems with inline not shared by macros?


There have been many discussions over the years on the macro vs inline
issue, and the result has always been that one should use macros in "C"
programming over inline for performance reasons.

The key elements are:
1)  inline functions are merely a *hint* to the compiler with no hard
     requirement to actually inline the code.
2)  inline functions do not optimize as fully as macros in all cases,
     since some of the compiler specific inline structuring can block
     some sorts of optimizations.
3)  Not all compilers support inlining, or do it even if they accept
     the syntax.

Thus there is no advantage to inlining in "C" programming. C++ amd other
object oriented languages use function calls extensively and inlining as
a way to partially offset the horrendous expense and slowdown this causes
for trivial methods. They also suffer from code bloat which is why the
inline operation is optional and often ignored by the compiler for more
complex inline cases. In "C" one has the macro preprocessor to handle all
these issues with no confusion about whether code is or is not inline and
is or is not being fully optimized.


Reducing macros to functions has been shown to slow Freeciv by over 50%.
What is the reason or justification for deprecating macros other than the
aesthetic sensibilities of a couple maintainers? This is a significant
change to fundamental Freeciv practice and clearly overthrowing the current
public programming model has not been well discussed with or received the
endorsement of the community.


As this patch introduces a concept (inline functions) that has so far
been pubically declared as undesirable for Freeciv, and it reduces
performance over the macro form, it should be rejected. When Freeciv moves
to C++ or another object oriented language where there is no preprocessor
advantage of "C" programming model, this could be revisited.

Cheers,
RossW
=====


Jason Short wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7195 >
> 
> The attached patch inlines map_pos_to_index.  This speeds up the server 
> by 5-10% as well as making the code smaller and more readible.
> 
> This is a special case of a piece of code that NEEDS to be 
> inlined/macrod, but for which macros are much inferior to inlining.  In 
> a discussion some weeks ago, the maintainers decided that inlining was 
> allowed but could not agree on the exact criteria for what should be 
> inlined.  We agreed that function-like macros and inlining should only 
> be used when it gives a helpful increase in overall speed - which 
> probably means most of our current macros should be (non-inline) 
> functions instead.  I think we all agreed this piece of code was a good 
> candidate for inlining.
> 
> jason
> 
> ------------------------------------------------------------------------
> 
> Index: common/map.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> retrieving revision 1.165
> diff -u -r1.165 map.h
> --- common/map.h      2003/11/28 17:37:21     1.165
> +++ common/map.h      2004/01/04 23:23:17
> @@ -222,21 +222,7 @@
>        *(pnat_x) = (2 * (map_x) - *(pnat_y) - (*(pnat_y) & 1)) / 2)          \
>     : (*(pnat_x) = (map_x), *(pnat_y) = (map_y)))
>  
> -/* Use map_to_native_pos instead unless you know what you're doing. */
> -#define map_pos_to_native_x(map_x, map_y)                                   \
> -  (topo_has_flag(TF_ISO)                                                    \
> -   ? (((map_x) - (map_y) + map.xsize                                        \
> -       - (((map_x) + (map_y) - map.xsize) & 1)) / 2)                        \
> -   : (map_x))
> -#define map_pos_to_native_y(map_x, map_y)                                   \
> -  (topo_has_flag(TF_ISO)                                                    \
> -   ? ((map_x) + (map_y) - map.xsize)                                        \
> -   : (map_y))
> -
> -#define map_pos_to_index(map_x, map_y)        \
> -  (CHECK_MAP_POS((map_x), (map_y)),           \
> -   (map_pos_to_native_x(map_x, map_y)         \
> -    + map_pos_to_native_y(map_x, map_y) * map.xsize))
> +static inline int map_pos_to_index(int map_x, int map_y);
>  
>  /* index_to_map_pos(int *, int *, int) inverts map_pos_to_index */
>  #define index_to_map_pos(pmap_x, pmap_y, index) \
> @@ -683,5 +669,21 @@
>  #define MAP_DEFAULT_SEPARATE_POLES   TRUE
>  #define MAP_MIN_SEPARATE_POLES       FALSE
>  #define MAP_MAX_SEPARATE_POLES       TRUE
> +
> +
> +/*
> + * Inline function definitions.  These are at the bottom because they may use
> + * elements defined above.
> + */
> +
> +static inline int map_pos_to_index(int map_x, int map_y)
> +{
> +  /* Note: writing this as a macro is hard; it needs temp variables. */
> +  int nat_x, nat_y;
> +
> +  CHECK_MAP_POS(map_x, map_y);
> +  map_to_native_pos(&nat_x, &nat_y, map_x, map_y);
> +  return nat_x + nat_y * map.xsize;
> +}
>  
>  #endif  /* FC__MAP_H */