From: Su Yue <l@damenly.su>
To: dsterba@suse.cz
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: make btrfs_node_key static inline
Date: Tue, 14 Sep 2021 22:55:03 +0800 [thread overview]
Message-ID: <35q719do.fsf@damenly.su> (raw)
In-Reply-To: <20210914143600.GB9286@suse.cz>
On Tue 14 Sep 2021 at 16:36, David Sterba <dsterba@suse.cz> wrote:
> On Tue, Sep 14, 2021 at 10:08:36PM +0800, Su Yue wrote:
>>
>> On Tue 14 Sep 2021 at 15:12, David Sterba <dsterba@suse.cz>
>> wrote:
>>
>> > On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:
>> >> It looks strange that btrfs_node_key is in struct-funcs.c.
>> >> So move it to ctree.h and make it static inline.
>> >
>> > "looks strange" is not a sufficient reason. Inlining a
>> > function
>> > means
>> > that the body will be expanded at each call site, bloating
>> > the
>> > binary
>> > code. Have you measured the impact of that?
>> >
>> Fair enough.
>>
>> Before:
>> text data bss dec hex filename
>> 1202418 123105 19384 1344907 14858b fs/btrfs/btrfs.ko
>> After:
>> text data bss dec hex filename
>> 1202620 123105 19384 1345109 148655 fs/btrfs/btrfs.ko
>>
>> +202
>>
>> > There's some performance cost of an non-inline function due
>> > to
>> > the call
>> > overhead but it does not make sense to inline a function
>> > that's
>> > called
>> > rarely and not in a tight loop. If you grep for the function
>> > you'd see
>> > that it's called eg. once per function or in a loop that's
>> > not
>> > performance critical on first sight (eg. in
>> > reada_for_search).
>>
>> Right, the patch won't impact performance obviously at the cost
>> of
>> +202 binary size. So I'd drop the patch.
>
> It does increase the size a bit but from what I've seen in the
> assembly
> it's not that bad and still probably worth doing the inline.
> There's one
> more extra call to read_extent_buffer (hidden behind
> read_eb_member
> macro).
>
Thanks for taking a look. I just noticed the lonely function then
post
the patch without deeper thinking.
> Cleaning up the node key helpers would be useful too, adding
> some
> more helpers and not calling read_eb_member in the end. I have a
> WIP
> patchset for that but had to leave it as there were bugs I did
> not find.
> I can bounce it to you if you're interested.
Thanks a lot. But I can't take it due to some reasons. Since you
have
a better WIP patchset, I don't mind forgoing the patch.
--
Su
prev parent reply other threads:[~2021-09-14 15:10 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-14 10:53 [PATCH] btrfs: make btrfs_node_key static inline Su Yue
2021-09-14 13:12 ` David Sterba
2021-09-14 14:08 ` Su Yue
2021-09-14 14:36 ` David Sterba
2021-09-14 14:55 ` Su Yue [this message]
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=35q719do.fsf@damenly.su \
--to=l@damenly.su \
--cc=dsterba@suse.cz \
--cc=linux-btrfs@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.