Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

common: hint the main branch of dout() accordingly to default verbosity. #21259

Merged
merged 1 commit into from
Apr 11, 2018
Merged

common: hint the main branch of dout() accordingly to default verbosity. #21259

merged 1 commit into from
Apr 11, 2018

Conversation

rzarzynski
Copy link
Contributor

@rzarzynski rzarzynski commented Apr 5, 2018 •
edited
Loading

Summary

In some cases the compiler arranges the dout-related code in a way unfriendly to statical branch prediction. That's is, the message crafting code is placed on the hot, fall-through path. The change modifies the behavior using __builtin_expect to hint the expected result accordingly to the default verbosity for the subsys being involved.

Before

00000000007fac70 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)>:
  7fac70:       41 57                   push   %r15
  7fac72:       41 56                   push   %r14
  7fac74:       49 89 ff                mov    %rdi,%r15
  7fac77:       41 55                   push   %r13
  7fac79:       41 54                   push   %r12
  7fac7b:       55                      push   %rbp
  7fac7c:       53                      push   %rbx
  7fac7d:       48 81 ec d8 02 00 00    sub    $0x2d8,%rsp
  7fac84:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
  7fac8b:       00 00 
  7fac8d:       48 89 84 24 c8 02 00    mov    %rax,0x2c8(%rsp)
  7fac94:       00 
  7fac95:       31 c0                   xor    %eax,%eax
  7fac97:       48 8b 47 28             mov    0x28(%rdi),%rax
  7fac9b:       48 89 94 24 80 00 00    mov    %rdx,0x80(%rsp)
  7faca2:       00 
  7faca3:       48 89 8c 24 90 00 00    mov    %rcx,0x90(%rsp)
  7facaa:       00 
  7facab:       4c 89 84 24 98 00 00    mov    %r8,0x98(%rsp)
  7facb2:       00 
  7facb3:       4c 89 8c 24 a0 00 00    mov    %r9,0xa0(%rsp)
  7facba:       00 
  7facbb:       48 8b 50 08             mov    0x8(%rax),%rdx
  7facbf:       80 ba df 02 00 00 13    cmpb   $0x13,0x2df(%rdx)
  7facc6:       0f 86 75 01 00 00       jbe    7fae41 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)+0x1d1>
  7faccc:       48 8b 78 10             mov    0x10(%rax),%rdi
  7facd0:       48 8d 0d d9 37 75 00    lea    0x7537d9(%rip),%rcx        # f4e4b0 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)::_log_exp_length>
  7facd7:       ba 2f 00 00 00          mov    $0x2f,%edx
  7facdc:       be 14 00 00 00          mov    $0x14,%esi
  7face1:       e8 6a 95 ab ff          callq  2b4250 <ceph::logging::Log::create_entry(int, int, unsigned long*)@plt>
  7face6:       48 89 c5                mov    %rax,%rbp
  7face9:       48 8b 40 78             mov    0x78(%rax),%rax
  7faced:       48 8d 35 c7 9a 3a 00    lea    0x3a9ac7(%rip),%rsi        # ba47bb <kstore_cnode_t::decode(ceph::buffer::list::iterator&)::__PRETTY_FUNCTION__+0x3b>
  7facf4:       ba 0a 00 00 00          mov    $0xa,%edx
  7facf9:       4d 8b 67 28             mov    0x28(%r15),%r12
  7facfd:       48 8d 58 48             lea    0x48(%rax),%rbx
  7fad01:       48 89 df                mov    %rbx,%rdi
  7fad04:       e8 17 96 ab ff          callq  2b4320 <std::basic_ostream<char, std::char_traits<char> >& std::__ostream_insert<char, std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*, long)@plt>

After

00000000007fd0d0 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)>:
  7fd0d0:       41 57                   push   %r15
  7fd0d2:       41 56                   push   %r14
  7fd0d4:       49 89 ff                mov    %rdi,%r15
  7fd0d7:       41 55                   push   %r13
  7fd0d9:       41 54                   push   %r12
  7fd0db:       55                      push   %rbp
  7fd0dc:       53                      push   %rbx
  7fd0dd:       48 81 ec d8 02 00 00    sub    $0x2d8,%rsp
  7fd0e4:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
  7fd0eb:       00 00 
  7fd0ed:       48 89 84 24 c8 02 00    mov    %rax,0x2c8(%rsp)
  7fd0f4:       00 
  7fd0f5:       31 c0                   xor    %eax,%eax
  7fd0f7:       48 8b 47 28             mov    0x28(%rdi),%rax
  7fd0fb:       48 89 94 24 80 00 00    mov    %rdx,0x80(%rsp)
  7fd102:       00 
  7fd103:       48 89 8c 24 90 00 00    mov    %rcx,0x90(%rsp)
  7fd10a:       00 
  7fd10b:       4c 89 84 24 98 00 00    mov    %r8,0x98(%rsp)
  7fd112:       00 
  7fd113:       4c 89 8c 24 a0 00 00    mov    %r9,0xa0(%rsp)
  7fd11a:       00 
  7fd11b:       48 8b 50 08             mov    0x8(%rax),%rdx
  7fd11f:       80 ba df 02 00 00 13    cmpb   $0x13,0x2df(%rdx)
  7fd126:       0f 87 cd 1e 00 00       ja     7feff9 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)+0x1f29>
  7fd12c:       48 8b bc 24 a0 00 00    mov    0xa0(%rsp),%rdi
  7fd133:       00 
  7fd134:       e8 17 3f bf ff          callq  3f1050 <ceph::buffer::list::clear()>
  7fd139:       48 8b 84 24 80 00 00    mov    0x80(%rsp),%rax
  7fd140:       00 
  7fd141:       48 8b 00                mov    (%rax),%rax
  7fd144:       48 8b 80 e8 00 00 00    mov    0xe8(%rax),%rax
  7fd14b:       48 3b 84 24 90 00 00    cmp    0x90(%rsp),%rax
  7fd152:       00 
  7fd153:       0f 86 4e 1a 00 00       jbe    7feba7 <BlueStore::_do_read(BlueStore::Collection*, boost::intrusive_ptr<BlueStore::Onode>, unsigned long, unsigned long, ceph::buffer::list&, unsigned int)+0x1ad7>
  7fd159:       f6 84 24 10 03 00 00    testb  $0x10,0x310(%rsp)

Signed-off-by: Radoslaw Zarzynski rzarzyns@redhat.com

rzarzynski
rzarzynski commented Apr 5, 2018
View reviewed changes
src/log/SubsystemMap.h
return LvlV <= static_cast<int>(m_gather_levels[SubV]);
// we expect that setting level different than the default
// is rather unusual.
return expect(LvlV <= static_cast<int>(m_gather_levels[SubV]),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being carried to the if (should_gather) in the dout_impl.

@rzarzynski rzarzynski assigned tchaikov and liewegas and unassigned tchaikov and liewegas Apr 5, 2018
@liewegas
Copy link
Member

liewegas commented Apr 9, 2018

retest this please

tchaikov
tchaikov reviewed Apr 10, 2018
View reviewed changes
src/log/SubsystemMap.h Outdated
// we expect that setting level different than the default
// is rather unusual.
return expect(LvlV <= static_cast<int>(m_gather_levels[SubV]),
LvlV <= ceph_subsys_get_max_default_level(SubV));;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, need to remove the trailing ";"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!

tchaikov
tchaikov approved these changes Apr 10, 2018
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from the nit, lgtm

@rzarzynski
common: hint the main branch of dout() accordingly to default verbosity.
bebf35a 
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
@tchaikov
Copy link
Contributor

@tchaikov tchaikov merged commit 3c31336 into ceph:master Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@liewegas liewegas Awaiting requested review from liewegas

Assignees
No one assigned
Labels
cleanup common performance wip-kefu-testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rzarzynski @liewegas @tchaikov