GNU bug report logs - #6734
"inline" overused in .c files?

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 6734 in the body.
You can then email your comments to 6734 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> CS.UCLA.EDU>
To: Bug-coreutils <bug-coreutils <at> gnu.org>
Cc: Bug-gnulib <bug-gnulib <at> gnu.org>, Chen Guo <chenguo4 <at> yahoo.com>
Subject: "inline" overused in .c files?
Date: Mon, 26 Jul 2010 11:53:36 -0700

I noticed thirteen "inline"s in coreutils/src/sort.c.  Just for fun, I
removed them all.  In ten cases, removing "inline" made no difference to
the generated machine code on my platform (RHEL 5, x86-64, GCC 4.1.2,
compiled with the typical gcc -O2).  In the three sort.c functions
that were exceptions (queue_insert, write_unique, check_insert),
removing "inline" made the overall code a tad shorter with no
measurable change to CPU performance.

Is there a reason those "inline"s are in there?  If not, I'm inclined
to remove them.  I can see a use for "static inline" in .h files, as
this asks the compiler not to warn about unused functions, but as far
as I know, it's typically not necessary to use "inline" in .c files
these days, as the compiler is typically smart enough.

I've checked this only for coreutils/src/sort.c but perhaps the same
argument applies to other source files in coreutils or other GNU apps, so
I'll CC: this to bug-gnulib for more-general comment.

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paolo Bonzini <bonzini <at> gnu.org>
To: Paul Eggert <eggert <at> CS.UCLA.EDU>
Cc: Bug-gnulib <bug-gnulib <at> gnu.org>, Bug-coreutils <bug-coreutils <at> gnu.org>,
	Chen Guo <chenguo4 <at> yahoo.com>
Subject: Re: "inline" overused in .c files?
Date: Mon, 26 Jul 2010 21:00:51 +0200

On 07/26/2010 08:53 PM, Paul Eggert wrote:
> I noticed thirteen "inline"s in coreutils/src/sort.c.  Just for fun, I
> removed them all.  In ten cases, removing "inline" made no difference to
> the generated machine code on my platform (RHEL 5, x86-64, GCC 4.1.2,
> compiled with the typical gcc -O2).  In the three sort.c functions
> that were exceptions (queue_insert, write_unique, check_insert),
> removing "inline" made the overall code a tad shorter with no
> measurable change to CPU performance.
>
> Is there a reason those "inline"s are in there?  If not, I'm inclined
> to remove them.  I can see a use for "static inline" in .h files, as
> this asks the compiler not to warn about unused functions, but as far
> as I know, it's typically not necessary to use "inline" in .c files
> these days, as the compiler is typically smart enough.

Only at -O3, except for special cases like functions called once. 
However, in general I agree that inline is not beneficial except for 
bottlenecks that can be identified by profiling.

Paolo

Message #11 received at 6734 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> CS.UCLA.EDU>
Cc: Bug-gnulib <bug-gnulib <at> gnu.org>, Chen Guo <chenguo4 <at> yahoo.com>,
	6734 <at> debbugs.gnu.org
Subject: Re: "inline" overused in .c files?
Date: Mon, 26 Jul 2010 23:47:06 +0100

On 26/07/10 19:53, Paul Eggert wrote:
> I noticed thirteen "inline"s in coreutils/src/sort.c.  Just for fun, I
> removed them all.  In ten cases, removing "inline" made no difference to
> the generated machine code on my platform (RHEL 5, x86-64, GCC 4.1.2,
> compiled with the typical gcc -O2).  In the three sort.c functions
> that were exceptions (queue_insert, write_unique, check_insert),
> removing "inline" made the overall code a tad shorter with no
> measurable change to CPU performance.
> 
> Is there a reason those "inline"s are in there?  If not, I'm inclined
> to remove them.  I can see a use for "static inline" in .h files, as
> this asks the compiler not to warn about unused functions, but as far
> as I know, it's typically not necessary to use "inline" in .c files
> these days, as the compiler is typically smart enough.
> 
> I've checked this only for coreutils/src/sort.c but perhaps the same
> argument applies to other source files in coreutils or other GNU apps, so
> I'll CC: this to bug-gnulib for more-general comment.

I never use inline in my own user space apps.
I do sometimes in lower level code when I want detailed control of the stack.
So please remove them.

cheers,
Pádraig.

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Bruno Haible <bruno <at> clisp.org>
To: bug-gnulib <at> gnu.org
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, Bug-coreutils <bug-coreutils <at> gnu.org>,
	Chen Guo <chenguo4 <at> yahoo.com>
Subject: Re: "inline" overused in .c files?
Date: Tue, 27 Jul 2010 01:22:07 +0200

Hi Paul,

In some cases, I experienced that a program can be made 5% faster just by
marking selected functions as 'inline'. Good candidates are those which
are only used at one place, and those which are small and where the function
call overhead would be noticeable.

In gnulib, we use 'inline' with moderation.

> this asks the compiler not to warn about unused functions

The absence of this warning when 'static inline' is used is not a problem,
because in this case gcc doesn't emit code for the unused function anyway.
I don't care much whether a .o file built by a compiler that doesn't
understand 'inline' is somewhat bigger, as this affects only few platforms,
like HP-UX. So I don't find your argument against the use of 'inline'
very strong.

But there are two other arguments against too much use of 'inline':
  1. It makes debugging harder, at least with many combinations of gcc + gdb.
  2. Denoting too many functions 'inline' could lead to not so good compiled
     code on x86 with gcc 2,/3.x. (Newer versions of gcc know how to save and
     restore a register even inside a function, where there is register
     pressure, so the use of 'inline' on large functions is not much of a
     problem any more.)

Bruno

Message #17 received at 6734 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> CS.UCLA.EDU>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: Bug-gnulib <bug-gnulib <at> gnu.org>, Chen Guo <chenguo4 <at> yahoo.com>,
	6734 <at> debbugs.gnu.org
Subject: Re: bug#6734: "inline" overused in .c files?
Date: Mon, 26 Jul 2010 18:45:12 -0700

After Bruno's comments it seems that some typical compilers
can benefit from "inline" on some functions, particularly small and
commonly used functions, so I removed just the "inline"s that didn't
appear like they would help measurably on any typical platform.

From 66e934b61f05ef32583df2a33f371c768b79c452 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Mon, 26 Jul 2010 18:38:19 -0700
Subject: [PATCH] sort: omit some "inline"s

* src/sort.c (mergelines, queue_destroy, queue_init, queue_insert):
(queue_pop, write_unique, mergelines_node, check_insert):
(update_parent): No longer inline; these uses of "inline"
seemed unlikely to help performance much.
---
 src/sort.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 1fd4ce7..2ee5a1d 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3007,7 +3007,7 @@ mergefiles (struct sortfile *files, size_t ntemps, size_t nfiles,
    T and LO point just past their respective arrays, and the arrays
    are in reverse order.  NLINES must be at least 2.  */
 
-static inline void
+static void
 mergelines (struct line *restrict t, size_t nlines,
             struct line const *restrict lo)
 {
@@ -3138,7 +3138,7 @@ unlock_node (struct merge_node *node)
 
 /* Destroy merge QUEUE. */
 
-static inline void
+static void
 queue_destroy (struct merge_node_queue *queue)
 {
   heap_free (queue->priority_queue);
@@ -3151,7 +3151,7 @@ queue_destroy (struct merge_node_queue *queue)
    RESERVE should accommodate all of them. Counting a NULL dummy head for the
    heap, RESERVE should be 2 * NTHREADS. */
 
-static inline void
+static void
 queue_init (struct merge_node_queue *queue, size_t reserve)
 {
   queue->priority_queue = heap_alloc (compare_nodes, reserve);
@@ -3162,7 +3162,7 @@ queue_init (struct merge_node_queue *queue, size_t reserve)
 /* Insert NODE into priority QUEUE. Assume caller either holds lock on NODE
    or does not need to lock NODE. */
 
-static inline void
+static void
 queue_insert (struct merge_node_queue *queue, struct merge_node *node)
 {
   pthread_mutex_lock (&queue->mutex);
@@ -3174,7 +3174,7 @@ queue_insert (struct merge_node_queue *queue, struct merge_node *node)
 
 /* Pop NODE off priority QUEUE. Guarantee a non-null, spinlocked NODE. */
 
-static inline struct merge_node *
+static struct merge_node *
 queue_pop (struct merge_node_queue *queue)
 {
   struct merge_node *node;
@@ -3192,7 +3192,7 @@ queue_pop (struct merge_node_queue *queue)
    this function does not actually save the line, nor any key information,
    thus is only appropriate for internal sort. */
 
-static inline void
+static void
 write_unique (struct line const *line, FILE *tfp, char const *temp_output)
 {
   static struct line const *saved = NULL;
@@ -3209,7 +3209,7 @@ write_unique (struct line const *line, FILE *tfp, char const *temp_output)
 /* Merge the lines currently available to a NODE in the binary
    merge tree, up to a maximum specified by MAX_MERGE. */
 
-static inline size_t
+static size_t
 mergelines_node (struct merge_node *restrict node, size_t total_lines,
                  FILE *tfp, char const *temp_output)
 {
@@ -3276,7 +3276,7 @@ mergelines_node (struct merge_node *restrict node, size_t total_lines,
 
 /* Insert NODE into QUEUE if it passes insertion checks. */
 
-static inline void
+static void
 check_insert (struct merge_node *node, struct merge_node_queue *queue)
 {
   size_t lo_avail = node->lo - node->end_lo;
@@ -3296,7 +3296,7 @@ check_insert (struct merge_node *node, struct merge_node_queue *queue)
 
 /* Update parent merge tree NODE. */
 
-static inline void
+static void
 update_parent (struct merge_node *node, size_t merged,
                struct merge_node_queue *queue)
 {
-- 
1.7.2

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Chen Guo <chen.guo.0625 <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Bug-gnulib <bug-gnulib <at> gnu.org>, Bug-coreutils <bug-coreutils <at> gnu.org>
Subject: Re: "inline" overused in .c files?
Date: Mon, 26 Jul 2010 21:29:33 -0700

On Mon, Jul 26, 2010 at 11:53 AM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> I noticed thirteen "inline"s in coreutils/src/sort.c.  Just for fun, I
> removed them all.  In ten cases, removing "inline" made no difference to
> the generated machine code on my platform (RHEL 5, x86-64, GCC 4.1.2,
> compiled with the typical gcc -O2).  In the three sort.c functions
> that were exceptions (queue_insert, write_unique, check_insert),
> removing "inline" made the overall code a tad shorter with no
> measurable change to CPU performance.
>
> Is there a reason those "inline"s are in there?  If not, I'm inclined
> to remove them.  I can see a use for "static inline" in .h files, as
> this asks the compiler not to warn about unused functions, but as far
> as I know, it's typically not necessary to use "inline" in .c files
> these days, as the compiler is typically smart enough.
>
> I've checked this only for coreutils/src/sort.c but perhaps the same
> argument applies to other source files in coreutils or other GNU apps, so
> I'll CC: this to bug-gnulib for more-general comment.
>

It's interesting that the three that are different are all from the new patch...

I can't think of a reason off the top of my head why we chose to inline them;
they are all short functions that are called _very_ often in our algorithm, and
that's the most likely reason why we inlined them.

I also recall a small bump in performance with at least write_unique, but that
could have just been a blip from testing on a shared machine.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.