Skip to content

common/log: Speed improvement for log#19100

Merged
tchaikov merged 2 commits intoceph:masterfrom
tchaikov:wip-log-reuse-streambuf
Dec 1, 2017
Merged

common/log: Speed improvement for log#19100
tchaikov merged 2 commits intoceph:masterfrom
tchaikov:wip-log-reuse-streambuf

Conversation

@tchaikov
Copy link
Copy Markdown
Contributor

@tchaikov tchaikov commented Nov 22, 2017 •
edited
Loading

@aclamk i adapted your PR of #17938 so it works with the rest part of logging system. also did some cleanup.

  • rename ThreadLocalPrebufferedStreambuf to CachedPrebufferedStreambuf, shorter name, and it makes the intention more visible.
  • move ostream out of CachedPrebufferedStreambuf. so it looks more reasonable: it's kind of weird at the first glance that a streambuf actually contains ostream!
  • let CachedPrebufferedStreambuf inherit from streambuf instead of basic_streambuf<>, less type
  • do not define the types for CachedPrebufferedStreambuf, as they are inherited from its parent class.
  • drop unused methods
  • use traits_type instead of traits_ty. the former is ready to use already.

@tchaikov tchaikov requested review from aclamk and cbodley November 22, 2017 15:01
@tchaikov tchaikov mentioned this pull request Nov 22, 2017
Closed
@tchaikov tchaikov force-pushed the wip-log-reuse-streambuf branch 3 times, most recently from c0abf5f to 2f31179 Compare November 22, 2017 15:10
@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Nov 23, 2017

before

[ RUN      ] Log.Speed_gather
[       OK ] Log.Speed_gather (1129 ms)
[ RUN      ] Log.Speed_nogather
[       OK ] Log.Speed_nogather (51 ms)

after

[ RUN      ] Log.Speed_gather
[       OK ] Log.Speed_gather (1484 ms)
[ RUN      ] Log.Speed_nogather
[       OK ] Log.Speed_nogather (58 ms)

@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Nov 23, 2017 •
edited
Loading

@tone-zhang the "make check" on aarch64 consistently fails recently. do you mind taking a look at your convenience?

@tone-zhang
Copy link
Copy Markdown
Contributor

tone-zhang commented Nov 23, 2017

@tchaikov My pleasure! I will have a look ASAP. Thanks!

@tone-zhang
Copy link
Copy Markdown
Contributor

tone-zhang commented Nov 24, 2017

@tchaikov I validated the PR and ran "make check -j16" command in my test bed, it looks everything is OK.

@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Nov 24, 2017

@tone-zhang thanks for verifying this!

probably this failure is not very reproducible, but in our jenkins slave, it happens frequently. see

@tone-zhang
Copy link
Copy Markdown
Contributor

tone-zhang commented Nov 24, 2017

@tchaikov All the above failures are in smoke.sh. I remember I have seen such error before.
In most of cases, when it appeared, it will disappear if I run it again. Thanks!

@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Nov 24, 2017

@aclamk
Copy link
Copy Markdown
Contributor

aclamk commented Nov 27, 2017 •
edited
Loading

@tchaikov

  1. rename ThreadLocalPrebufferedStreambuf to CachedPrebufferedStreambuf, shorter name, and it makes the intention more visible.
    +1 You are probably right. Being thread-local is just a tool to reduce execution time.
  2. move ostream out of CachedPrebufferedStreambuf. so it looks more reasonable: it's kind of weird at the first glance that a streambuf actually contains ostream!
    I am not 100% sure about that. Sure, ostream as a member looks wierd. But lifetimes of m_streambuf and m_os are closely coupled. Hiding ostream inside m_streambuf enforces this requirement.
  3. let CachedPrebufferedStreambuf inherit from streambuf instead of basic_streambuf<>, less type
    +1
  4. do not define the types for CachedPrebufferedStreambuf, as they are inherited from its parent class.
    +1
  5. drop unused methods
    +1
  6. use traits_type instead of traits_ty. the former is ready to use already.
    +1

@aclamk @tchaikov
common/log: Performance test for logging.
2c332eb 
Signed-off-by: Adam Kupczyk <akupczyk@redhat.com>
@tchaikov tchaikov force-pushed the wip-log-reuse-streambuf branch from 2f31179 to 27bd214 Compare November 28, 2017 06:03
@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Nov 28, 2017 •
edited
Loading

@aclamk i reverted the second change, and rebased against master. could you take another look?

@aclamk @tchaikov
common/log: Added new version of PrebufferedStreambuf, tuned for log …
b08ae65 
…usage

Signed-off-by: Adam Kupczyk <akupczyk@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov tchaikov force-pushed the wip-log-reuse-streambuf branch from 27bd214 to b08ae65 Compare November 28, 2017 08:15
aclamk
aclamk approved these changes Nov 30, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@aclamk aclamk left a comment

Choose a reason for hiding this comment

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

Before:
[ RUN ] Log.Speed_gather
[ OK ] Log.Speed_gather (1625 ms)
[ RUN ] Log.Speed_nogather
[ OK ] Log.Speed_nogather (52 ms)

After
[ RUN ] Log.Speed_gather
[ OK ] Log.Speed_gather (1273 ms)
[ RUN ] Log.Speed_nogather
[ OK ] Log.Speed_nogather (45 ms)

Everything looks good.

@tchaikov
Copy link
Copy Markdown
Contributor Author

tchaikov commented Dec 1, 2017

@tchaikov tchaikov merged commit 704b6db into ceph:master Dec 1, 2017
@tchaikov tchaikov deleted the wip-log-reuse-streambuf branch December 1, 2017 02:34
This was referenced Jun 24, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbodley cbodley cbodley approved these changes

@aclamk aclamk aclamk approved these changes

Assignees

No one assigned

Labels

common needs-qa needs-review performance wip-kefu-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tchaikov @tone-zhang @aclamk @cbodley