Patch: Remove gcc dependency in definition of inline functions

Kurt Harriman <harriman@acm.org> writes:

(Does anybody still use a C compiler that doesn't support
inline functions?)

The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc. At minimum that
would require
* not generating extra copies of the function
* not whining about unreferenced static functions
How many compilers have you tested this patch against? Which ones
does it actually offer any benefit for?

regards, tom lane

On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Kurt Harriman <harriman@acm.org> writes:

(Does anybody still use a C compiler that doesn't support
inline functions?)

+1 for modern C.

The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc. At minimum that
would require
* not generating extra copies of the function
* not whining about unreferenced static functions
How many compilers have you tested this patch against? Which ones
does it actually offer any benefit for?

Those are not correctness problems. Compilers with non-optimal or
missing 'inline' do belong together with compilers without working int64.
We may spend some effort to be able to compile on them, but they
should not affect our coding style.

'static inline' is superior to macros because of type-safety and
side-effects avoidance. I'd suggest event removing any HAVE_INLINE
ifdefs and let autoconf undef the 'inline' if needed. Less duplicated
code to maintain. The existence of compilers in active use without
working 'inline' seems quite hypothetical these days.

--
marko

Marko Kreen wrote:

On 11/29/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Kurt Harriman <harriman@acm.org> writes:

(Does anybody still use a C compiler that doesn't support
inline functions?)

+1 for modern C.

The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc. At minimum that
would require
* not generating extra copies of the function
* not whining about unreferenced static functions
How many compilers have you tested this patch against? Which ones
does it actually offer any benefit for?

Those are not correctness problems. Compilers with non-optimal or
missing 'inline' do belong together with compilers without working int64.
We may spend some effort to be able to compile on them, but they
should not affect our coding style.

'static inline' is superior to macros because of type-safety and
side-effects avoidance. I'd suggest event removing any HAVE_INLINE
ifdefs and let autoconf undef the 'inline' if needed. Less duplicated
code to maintain. The existence of compilers in active use without
working 'inline' seems quite hypothetical these days.

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On mån, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

Sure, but one could argue that a compiler that doesn't support inline
usefully is probably not the sort of compiler that you use for compiling
performance-relevant software anyway. We can support such systems in a
degraded way for historical value and evaluation purposes as long as
it's pretty much free, like we support systems without working int8.

Peter Eisentraut wrote:

On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

Sure, but one could argue that a compiler that doesn't support inline
usefully is probably not the sort of compiler that you use for compiling
performance-relevant software anyway. We can support such systems in a
degraded way for historical value and evaluation purposes as long as
it's pretty much free, like we support systems without working int8.

The issue is that many compilers will take "inline" as a suggestion and
decide if it is worth-while to inline it --- I don't think it is inlined
unconditionally by any modern compilers.

Right now we think we are better at deciding what should be inlined than
the compiler --- of course, we might be wrong, and it would be good to
performance test this.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On 11/30/09, Bruce Momjian <bruce@momjian.us> wrote:

Peter Eisentraut wrote:

On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

Sure, but one could argue that a compiler that doesn't support inline
usefully is probably not the sort of compiler that you use for compiling
performance-relevant software anyway. We can support such systems in a
degraded way for historical value and evaluation purposes as long as
it's pretty much free, like we support systems without working int8.

The issue is that many compilers will take "inline" as a suggestion and
decide if it is worth-while to inline it --- I don't think it is inlined
unconditionally by any modern compilers.

Right now we think we are better at deciding what should be inlined than
the compiler --- of course, we might be wrong, and it would be good to
performance test this.

Note - my proposal would be to get rid of HAVE_INLINE, which
means we are already using inline functions unconditionally
on platforms that matter (gcc). Keeping duplicate code
for obsolete compilers is pointless.

I'm not suggesting converting all existing macros to inline. This
can happen slowly, and where it brings benefit (short but multi-arg
are probably most worthwhile to convert). Also new macros are better
done as inlines.

About uninlining - usually if the compiler decides to uninline,
it is probably right anyway. The prime example would be Linux kernel
where the 'inline' is used quite a lot as go-faster-magic-dust on totally
random functions. (In .c files, not talking about headers)

Most compilers (gcc, vc, icc) support also force-inlining, which is
not taken as hint but command. If you want to be on safe side, you
could define 'inline' as force-inline on compilers that support that.

--
marko

Marko Kreen wrote:

Note - my proposal would be to get rid of HAVE_INLINE, which
means we are already using inline functions unconditionally
on platforms that matter (gcc). Keeping duplicate code
for obsolete compilers is pointless.

Microsoft C doesn't matter?

I seem to remember that when the Win32 version became available it
actually increased the
number of people trying postgres rather dramatically. Did that count
for nothing?

James Mansion <james@mansionfamily.plus.com> writes:

Marko Kreen wrote:

Note - my proposal would be to get rid of HAVE_INLINE, which
means we are already using inline functions unconditionally
on platforms that matter (gcc). Keeping duplicate code
for obsolete compilers is pointless.

Microsoft C doesn't matter?

Breaking compilers that don't have inline at all isn't happening;
it wouldn't buy us anything much anyway. The debate here is about
how much we can assume about the behavior of compilers that do
recognize the keyword. In particular, do they behave sensibly
when finding an unreferenced static inline function, which is what
would occur in many modules if we allow them to see inline functions
in headers.

regards, tom lane

On 12/2/09, James Mansion <james@mansionfamily.plus.com> wrote:

Marko Kreen wrote:

Note - my proposal would be to get rid of HAVE_INLINE, which
means we are already using inline functions unconditionally
on platforms that matter (gcc). Keeping duplicate code
for obsolete compilers is pointless.

Microsoft C doesn't matter?

I seem to remember that when the Win32 version became available it actually
increased the
number of people trying postgres rather dramatically. Did that count for
nothing?

The "(gcc)" above meant the inline functions are already used with gcc.

I have no reason to think Microsoft's inlining works worse than gcc's.

IOW - if the compiler does not support 'static inline' we should fall back
to plain 'static' functions, instead maintaining duplicate macros.
Such compilers would take a efficiency hit, but as they are practically
non-existent they dont matter.

Microsoft C does support inline, so it would not be affected.

--
marko

Tom Lane wrote:

Kurt Harriman <harriman@acm.org> writes:

(Does anybody still use a C compiler that doesn't support
inline functions?)

The question isn't so much that, it's whether the compiler supports
inline functions with the same behavior as gcc.

With this patch, if the compiler doesn't accept the "inline" keyword
(or __inline or __inline__), then HAVE_INLINE remains undefined, and
the out-of-line definitions are used.

So, these concerns would be applicable in case there is a compiler
which accepts the keyword but ignores it, thus fooling "configure".

(Also these concerns become applicable if we eliminate the
out-of-line definitions altogether as some have suggested.)

At minimum that would require
* not generating extra copies of the function

Even if someone uses such a compiler, maybe the extra copies are not
too bad. These are small functions. If not inlined, the compiled
code size on x86-32 is
list_head 48 bytes
list_tail 48 bytes
list_length 48 bytes
MemoryContextSwitchTo 32 bytes
Total 176 bytes
In src/backend there are 478 .c files that #include postgres.h, so the
potential bloat is about 82 KB (= 478 * 176).

This would also occur anytime the user specifies compiler options that
suppress inlining, such as for a debug build; but then the executable
size doesn't matter usually.

* not whining about unreferenced static functions

I could update the patch so that "configure" will test for this.

(BTW, MSVC is a whiner, but clams up if __forceinline is used.)

How many compilers have you tested this patch against?

Only a small number. By submitting it to pgsql-hackers, I hope that
it can be exposed to broader coverage.

Which ones does it actually offer any benefit for?

MSVC is one.

I hope eventually it will be found that all compilers of interest
do a good enough job with static inline functions so that we can
use a lot more of them. For example, I'd like to expunge the
abominable heap_getattr() and fastgetattr() macros in access/htup.h
and replace them with an inline function. Such improvements are
less easily justifiable if they are limited to gcc.

Regards,
... kurt

Bruce Momjian wrote:

Peter Eisentraut wrote:

On m?n, 2009-11-30 at 07:06 -0500, Bruce Momjian wrote:

I thought one problem was that inline is a suggestion that the compiler
can ignore, while macros have to be implemented as specified.

Sure, but one could argue that a compiler that doesn't support inline
usefully is probably not the sort of compiler that you use for compiling
performance-relevant software anyway. We can support such systems in a
degraded way for historical value and evaluation purposes as long as
it's pretty much free, like we support systems without working int8.

The issue is that many compilers will take "inline" as a suggestion and
decide if it is worth-while to inline it --- I don't think it is inlined
unconditionally by any modern compilers.

Right now we think we are better at deciding what should be inlined than
the compiler --- of course, we might be wrong, and it would be good to
performance test this.

Just to clarify, this patch doesn't add new inline functions or
convert any macros to inline functions. At present, PostgreSQL
header files define four functions as inline for gcc only. This
patch merely opens up those existing inline functions to be used
with any compiler that accepts them, not just gcc. If this goes
well, it may be a preparatory step toward future adoption of more
inline functions and fewer macros.

Regards,
... kurt

Hi,

Attached is a revised patch, offered for the 2010-01 commitfest.
It's also available in my git repository in the "submitted" branch:
http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

In this version, the "configure" script tests whether a static
inline function can be defined without incurring a warning when
not referenced. If successful, the preprocessor symbol PG_INLINE
is defined in pg_config.h to the appropriate keyword: inline,
__inline, __inline__, or __forceinline. Otherwise PG_INLINE
remains undefined.

palloc.h and pg_list.h condition their inline function
definitions on PG_INLINE instead of the gcc-specific __GNUC__.
Thus the functions can be inlined on more platforms, not only
gcc.

Ordinary out-of-line calls are still used if the compiler doesn't
recognize inline functions, or spews warnings when static inline
functions are defined but not referenced.

Regards,
... kurt

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:

Attached is a revised patch, offered for the 2010-01 commitfest.
It's also available in my git repository in the "submitted" branch:

http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

In this version, the "configure" script tests whether a static
inline function can be defined without incurring a warning when
not referenced. If successful, the preprocessor symbol PG_INLINE
is defined in pg_config.h to the appropriate keyword: inline,
__inline, __inline__, or __forceinline. Otherwise PG_INLINE
remains undefined.

palloc.h and pg_list.h condition their inline function
definitions on PG_INLINE instead of the gcc-specific __GNUC__.
Thus the functions can be inlined on more platforms, not only
gcc.

Ordinary out-of-line calls are still used if the compiler doesn't
recognize inline functions, or spews warnings when static inline
functions are defined but not referenced.

-1. The PG_INLINE is ugly.

In actual C code we should see only "inline" keyword, no XX_INLINE,
__inline, __inline__, etc. It's up to configure to make sure "inline"
is something that C compiler accepts or simply defined to empty string
otherwise.

I assume you are playing with force-inline to avoid warnings on some
compiler? Do you a actual compiler that behaves like that? Unless
it is some popular compiler (as "in actual use") it is premature
complexity. Please remove that.

We may want to have force-inline in the future, when we start converting
some more complex macros to inline functions, but then it should cover
all main compilers, and current patch does not try to do it.

So my suggestions:

1. Make sure "inline" is defined, empty or to something that works.
(plain AC_C_INLINE seems to do that)
2. Convert current __inline, __inline__ sites to "inline"

and

3. Remove #ifdefs and duplicate macros, keeping only inline funcs.

There does not seem to be any holding counter-arguments why we should
not do that, so lets go ahead?

--
marko

On 12/15/2009 4:05 AM, Marko Kreen wrote:

On 12/15/09, Kurt Harriman<harriman@acm.org> wrote:

Attached is a revised patch, offered for the 2010-01 commitfest.
It's also available in my git repository in the "submitted" branch:

http://git.postgresql.org/gitweb?p=users/harriman/share.git;a=shortlog;h=refs/heads/submitted

-1. The PG_INLINE is ugly.

In actual C code we should see only "inline" keyword, no XX_INLINE,
__inline, __inline__, etc. It's up to configure to make sure "inline"
is something that C compiler accepts or simply defined to empty string
otherwise.

I assume you are playing with force-inline to avoid warnings on some
compiler? Do you a actual compiler that behaves like that? Unless
it is some popular compiler (as "in actual use") it is premature
complexity. Please remove that.

We may want to have force-inline in the future, when we start converting
some more complex macros to inline functions, but then it should cover
all main compilers, and current patch does not try to do it.

So my suggestions:

1. Make sure "inline" is defined, empty or to something that works.
(plain AC_C_INLINE seems to do that)
2. Convert current __inline, __inline__ sites to "inline"

and

3. Remove #ifdefs and duplicate macros, keeping only inline funcs.

There does not seem to be any holding counter-arguments why we should
not do that, so lets go ahead?

Hi Marko,

Thanks for your comments.

Although I agree that the nicest code would be as you suggest, and
that would work fine with gcc, I hesitate to dismiss so quickly the
counter-arguments:

i) That would reduce the number of platforms on which the
backend compiles cleanly with no warnings. Microsoft C
platforms would be among those affected.

ii) Your item #3 would unnecessarily increase the size of
the postgres executable on platforms where no inline keyword
is recognized and unused static functions aren't optimized
away. The increase would be about 82 KB if this were to
occur on an x86-32 platform, for example. Maybe nobody still
uses such an old compiler, or if they do, maybe the increased
executable size would be small enough to ignore; but I don't
know this. Users who would be affected are not necessarily
readers of the pgsql-hackers list.

This patch is meant to broaden the number of platforms benefiting
from inlining. Also it is hoped to facilitate greater use of inline
functions in the future, in preference to macros. I don't want to
disadvantage the older platforms, unless they are unambiguously not
important anymore.

-1. The PG_INLINE is ugly.

Yes, but not quite as ugly as the existing code. It is more
portable, eliminating hard coded gcc dependencies in several
places.

1. Make sure "inline" is defined, empty or to something that works.
(plain AC_C_INLINE seems to do that)

Yes, it is already that way in the existing code. If the compiler
doesn't accept plain "inline", then the "configure" script sets up
pg_config.h to #define inline as a preprocessor symbol that maps to
__inline, __inline__, or empty. As you say, plain AC_C_INLINE takes
care of that. My patch doesn't affect this.

In actual C code we should see only "inline" keyword, no XX_INLINE,
__inline, __inline__, etc. It's up to configure to make sure "inline"
is something that C compiler accepts or simply defined to empty string
otherwise.

Yes, that is already how we define inline functions in .c files
such as executor/nodeSetOp.c, optimizer/path/joinpath.c, and
storage/lmgr/lock.c. It seems to work fine there.

However, there is a difference between .c files and .h files.

A static inline definition in a .c file is certain to be referenced,
and is never instantiated more than once even if not inlined.

Therefore in .c files we need not be concerned about the two issues
mentioned above: superfluous "defined but not used" warnings, and
unnecessary bloating of the executable.

On the other hand, a static inline definition in a .h file is
liable to be #included into (perhaps hundreds of) compilations
where it is not referenced. Some compilers conscientiously
display a warning every time this happens. Some emit
out-of-line code every time they see the definition, even if
it is not called (likeliest when inline is not supported at all
and is #defined to empty).

To avoid these problems, historically the PostgreSQL developers
have minimized the use of inline functions in header files, and
instead used macros. In the two header files where macros were
considered too ugly, inline functions were conditionally
compiled for gcc only. At that time, __inline__ was a
nonstandard gcc extension.

Nowadays, inline functions are part of standard C99 and C++.
For static inline functions, the C99 and C++ standard behavior
is basically the same as the old gcc extension. So by now,
most compilers support this somehow, and it makes sense to
remove the gcc dependency.

I assume you are playing with force-inline to avoid warnings on some
compiler? Do you a actual compiler that behaves like that?

Yes, Microsoft compilers warn about unreferenced static
functions, but keep quiet when the function is defined with
__forceinline.

Inline functions defined in header files are typically small
enough that compilers will always obey the inline directive,
so it doesn't matter whether plain inline or __forceinline is
used, except for the warnings. This is certainly the case for
the four inline functions which exist at present in palloc.h
and pg_list.h.

So far in my experience outside PostgreSQL, compilers almost
always inline everything that I have defined as inline. I've
never had occasion to use __forceinline with a modern compiler,
except now for the MSVC warnings.

Unless it is some popular compiler (as "in actual use") it is
premature complexity. Please remove that.

Microsoft's compilers are in actual use, and some might even
say they are popular. For example, James Mansion posted to
this effect on 2 December.

We may want to have force-inline in the future, when we start converting
some more complex macros to inline functions, but then it should cover
all main compilers, and current patch does not try to do it.

At present I do not know of any reason why we should want
__forceinline, except here for its side-effect of silencing
MSVC's warnings about static functions that are defined but
not used.

Instead of using __forceinline, it would be possible to use
CFLAGS or a #pragma to globally suppress the unreferenced
symbol warnings. However, the warning codes seem less
portable because they may be specific to one compiler or
change between releases. Also, useful warnings might be
hidden along with the superfluous ones.

Regards,
... kurt

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:

On 12/15/2009 4:05 AM, Marko Kreen wrote:

Unless it is some popular compiler (as "in actual use") it is
premature complexity. Please remove that.

Microsoft's compilers are in actual use, and some might even
say they are popular. For example, James Mansion posted to
this effect on 2 December.

Erm, AFAIK he simply misunderstood my "(gcc)" comment.

Do you have actual proof that MSVC launches warnings on unused
"static inline" functions? Not "static", but "static inline".

If yes, indeed we need to fix it. MSVC is broken then, but it does
not matter as we need to work well on it. We can fix it with either
force-inline, or equivalent with gcc's __attribute__((unused)).

If no, then we don't need to fix it. Adding complexity based on
some email miscommunication seems wrong to me...

--
marko

On 12/15/2009 1:31 PM, Marko Kreen wrote:

Do you have actual proof that MSVC launches warnings on unused
"static inline" functions? Not "static", but "static inline".

Yes, I tried it, and that's why I did it this way.

If yes, indeed we need to fix it. MSVC is broken then, but it does
not matter as we need to work well on it. We can fix it with either
force-inline, or equivalent with gcc's __attribute__((unused)).

Microsoft doesn't support the gcc __attribute__ syntax, AFAIK.

They have a few other gewgaws, like __declspec__, but I didn't
find one that helps with this problem.

Regards,
... kurt

On 12/15/09, Kurt Harriman <harriman@acm.org> wrote:

On 12/15/2009 1:31 PM, Marko Kreen wrote:

Do you have actual proof that MSVC launches warnings on unused
"static inline" functions? Not "static", but "static inline".

Yes, I tried it, and that's why I did it this way.

Oh. Ok then. Force-inline seems better fix as we may want to use
it for other reasons too (converting big macros).

So it seems like a good moment to solve it for gcc too.

Your worry ii) can be ignored, managing to compile on such
compilers is already overachievement.

If yes, indeed we need to fix it. MSVC is broken then, but it does
not matter as we need to work well on it. We can fix it with either
force-inline, or equivalent with gcc's __attribute__((unused)).

Microsoft doesn't support the gcc __attribute__ syntax, AFAIK.

They have a few other gewgaws, like __declspec__, but I didn't
find one that helps with this problem.

The question is now what should we put into configure and what into
headers.

Simplest would be to have plain AC_C_INLINE in configure
and then in header (c.h?):

#ifdef _MSC_VER
#undef inline
#define inline __forceinline
#else
#ifdef __GNUC__
#undef inline
#define inline inline __attribute__((always_inline))
#endif
#endif

(Not compile tested :)

Or should we put that logic also into configure,
so that the config.h already contains proper 'inline'?

Although fitting it into win32 buildsystem seems to be bit more
complex in that case...

--
marko

On 12/15/2009 2:09 PM, Marko Kreen wrote:

Oh. Ok then. Force-inline seems better fix as we may want to use
it for other reasons too (converting big macros).

So it seems like a good moment to solve it for gcc too.

Is ordinary inlining not sufficient?

If deluxe inlining is something that might be wanted in the
future, but isn't needed right now, perhaps we should wait
until then.

Your worry ii) can be ignored, managing to compile on such
compilers is already overachievement.

I think so too. With your opinion added to mine, do we constitute a
consensus of the pg community? Someone might object that a sample of
two individuals is insufficiently representative of the whole, but
away with the pedants: let us not quibble over trifles.

The question is now what should we put into configure and what into
headers.

PostgreSQL seems mostly to follow the GNU

[Please ignore the previous incomplete version of this reply, which I
sent by mistake. Sorry for the list noise.]

On 12/15/2009 2:09 PM, Marko Kreen wrote:

Oh. Ok then. Force-inline seems better fix as we may want to use
it for other reasons too (converting big macros).

So it seems like a good moment to solve it for gcc too.

Is ordinary inlining not sufficient?

If deluxe inlining is something that might be wanted in the
future, but isn't needed right now, perhaps we should wait
until then.

Your worry ii) can be ignored, managing to compile on such
compilers is already overachievement.

I think so too. With your opinion added to mine, do we constitute a
consensus of the pg community? Someone might object that a sample of
two individuals is insufficiently representative of the whole, but
away with the pedants: let us not quibble over trifles.

The question is now what should we put into configure and what into
headers.

PostgreSQL seems mostly to follow the GNU tradition of using
autoconf rather than thickets of #ifdefs to discover platform
characteristics such as supported features, variant spellings of
keywords and identifiers, and the like. This often makes it easier
to support new platforms, assuming the autoconf mechanism is
understood.

Autoconf facilitates testing directly for the needed features:
generally a better approach than hard-coding knowledge of the
peculiarities of particular compilers, compiler release levels,
instruction set architectures, etc. For example, instead of
writing #ifdefs to decide, "if this is gcc, and the version is
high enough, then __funcname__ should work", it's better to let
autoconf actually try to compile a program using __funcname__.
That way PostgreSQL can keep up with the evolution of improved
and new compilers without continually updating an #ifdef-encoded
knowledge base of compiler capabilities.

Simplest would be to have plain AC_C_INLINE in configure
and then in header (c.h?):

#ifdef _MSC_VER
#undef inline
#define inline __forceinline
#else
#ifdef __GNUC__
#undef inline
#define inline inline __attribute__((always_inline))
#endif
#endif

(Not compile tested :)

This would force every inline. Why do we want that?

For gcc, I think the __attribute__ has to come after the function's
parameter list, rather than before the return type.

Or should we put that logic also into configure,
so that the config.h already contains proper 'inline'?

That's how it works at present for plain (not forced) inline.

Regards,
... kurt