public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: David Chinner <dgc@sgi.com>
To: David Chatterton <chatz@melbourne.sgi.com>
Cc: David Chinner <dgc@sgi.com>,
	Russell Cattelan <cattelan@thebarn.com>,
	Tim Shimmin <tes@sgi.com>, Eric Sandeen <sandeen@sandeen.net>,
	xfs@oss.sgi.com
Subject: Re: [PATCH 1/2] Make stuff static
Date: Wed, 29 Nov 2006 18:31:34 +1100	[thread overview]
Message-ID: <20061129073134.GD33919298@melbourne.sgi.com> (raw)
In-Reply-To: <4563D7DD.1060907@melbourne.sgi.com>

On Wed, Nov 22, 2006 at 03:53:49PM +1100, David Chatterton wrote:
> David Chinner wrote:
> > Attached is thecomplete patch. On ia64, the size of the xfs.ko
> > and xfs_quota.ko modules decreases with this patch:
....
> > Performance appears to be slight faster with the noinline
> > patch, but the variation is within the error margins of
> > my measurements so I'd say it's neutral.
> > 
> > Comments?
> > 
> 
> Just reducing xfs_bmapi by 118 bytes makes this worthwhile doesn't it?

Well, this is ia64, so stack usage really isn't the issue there.
A performance degradation was really what I really care about with
this change on ia64.

And the increase in xfs_bmap_btalloc() offsets this saving as well....

> Out of interest, what estimated improvement does this have on one of Jesper's
> stacks?

Depends on the compiler. For gcc 3.3.5, it makes no difference at
all because it doesn't automatically inline static functions. The
problem we're trying to address here is the agressive inlining that
gcc 4.x does of static functions that increases the stack usage
of critical functions.

e.g we've got in the code:

	xfs_bmapi()
	  xfs_bmap_alloc()
	    xfs_bmap_btalloc()

xfs_bmap_{bt}alloc() are static, single use functions, and so gcc 4.x
inlines them and the stack usage of all three functions is brought
into xfs_bmapi().

Now in some cases this is a slight win in terms in stack usage
if the code always passes through that path, but if the inlined
functions are leaf functions, then we increase stack usage for
no gain.

The clearest example of this on i386 is xfs_page_state_convert(),
which goes from 368 bytes of stack usage to 160 bytes of stack usage
once noinline is used on i386. There's over 200 bytes of extra stack
used by inlining functions that are not in the critical path.

Basically, we can factor the code all we like, but while gcc is
undoing that factoring to "go fast" we are fighting a losing battle.
Hence the noinline change is needed first to make the code stack how
we want it to, not how the compiler thinks it should.

> Should we be concerned that there are now more functions with 100 or more bytes?

Not really - the work that Jesper and Keith have done shows us that
we've got a certain set of functions that we really need to
concentrate on and once we've dealt with them we can start looking
at other leaf functions that are stack hogs....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

  parent reply	other threads:[~2006-11-29  7:32 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-09-29  3:28 [PATCH 1/2] Make stuff static sandeen
2006-10-14  4:31 ` Eric Sandeen
2006-10-16  9:12 ` Timothy Shimmin
2006-10-16 13:49   ` Eric Sandeen
2006-10-16 21:34     ` Eric Sandeen
2006-10-16 23:22       ` David Chinner
2006-10-16 23:55         ` Russell Cattelan
2006-10-17  0:50           ` David Chinner
2006-10-17  1:03             ` Eric Sandeen
2006-10-17  3:09               ` David Chinner
2006-10-17  3:18                 ` Nathan Scott
2006-10-18  0:56               ` David Chinner
2006-10-17  7:13             ` Tim Shimmin
2006-10-17 21:57               ` David Chinner
2006-10-17 22:45                 ` Russell Cattelan
2006-11-22  0:42                   ` David Chinner
2006-11-22  1:09                     ` Russell Cattelan
2006-11-22  2:16                       ` David Chatterton
2006-11-22  4:24                       ` David Chinner
2006-11-22  4:53                         ` David Chatterton
2006-11-22 16:13                           ` Eric Sandeen
2006-11-29  7:31                           ` David Chinner [this message]
2006-11-26 14:05                         ` Eric Sandeen
2006-10-18  4:06                 ` Timothy Shimmin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20061129073134.GD33919298@melbourne.sgi.com \
    --to=dgc@sgi.com \
    --cc=cattelan@thebarn.com \
    --cc=chatz@melbourne.sgi.com \
    --cc=sandeen@sandeen.net \
    --cc=tes@sgi.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox