| Message ID | 20210914105335.28760-1-l@damenly.su (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: make btrfs_node_key static inline | expand |
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? 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).
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. Sorry for the noise. -- Su
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). 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.
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
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 452e410adbf5..67340a74f9ed 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1939,8 +1939,14 @@ static inline unsigned long btrfs_node_key_ptr_offset(int nr) sizeof(struct btrfs_key_ptr) * nr; } -void btrfs_node_key(const struct extent_buffer *eb, - struct btrfs_disk_key *disk_key, int nr); +static inline void btrfs_node_key(const struct extent_buffer *eb, + struct btrfs_disk_key *disk_key, int nr) +{ + unsigned long ptr = btrfs_node_key_ptr_offset(nr); + read_eb_member(eb, (struct btrfs_key_ptr *)ptr, + struct btrfs_key_ptr, key, disk_key); + +} static inline void btrfs_set_node_key(const struct extent_buffer *eb, struct btrfs_disk_key *disk_key, int nr) diff --git a/fs/btrfs/struct-funcs.c b/fs/btrfs/struct-funcs.c index f429256f56db..7526005525cb 100644 --- a/fs/btrfs/struct-funcs.c +++ b/fs/btrfs/struct-funcs.c @@ -161,11 +161,3 @@ DEFINE_BTRFS_SETGET_BITS(8) DEFINE_BTRFS_SETGET_BITS(16) DEFINE_BTRFS_SETGET_BITS(32) DEFINE_BTRFS_SETGET_BITS(64) - -void btrfs_node_key(const struct extent_buffer *eb, - struct btrfs_disk_key *disk_key, int nr) -{ - unsigned long ptr = btrfs_node_key_ptr_offset(nr); - read_eb_member(eb, (struct btrfs_key_ptr *)ptr, - struct btrfs_key_ptr, key, disk_key); -}
It looks strange that btrfs_node_key is in struct-funcs.c. So move it to ctree.h and make it static inline. Signed-off-by: Su Yue <l@damenly.su> --- fs/btrfs/ctree.h | 10 ++++++++-- fs/btrfs/struct-funcs.c | 8 -------- 2 files changed, 8 insertions(+), 10 deletions(-)