osd: reduce string use in coll_t::calc_str() #6505

aiicore · 2015-11-09T14:15:46Z

This pull request should be treated as a concept of reducing string usage in well known fixed-scenarios. Functions dec2char and hex2char are faster than << hex << and sprintf() but less secure (they don't know anything about buffer size, and could easily cause buffer overflow).

Using those functions in separate C code (PoC) caused 11x reduction in cpu user time vs std::string and 2x vs sprintf() with the same pattern.

Running test on whole cluster caused cpu user time reduction, mostly for small write size and big number of threads/objects. Cpu time reduction is noticeable not significant, maybe touching similar places in code could cause bigger impact.

Code for deprecated options was added for compatibility.

Signed-off-by: Igor Podoski igor.podoski@ts.fujitsu.com

liewegas · 2015-11-09T14:47:12Z

Looks reasonable to me. It might make sense to move part of this into [s]pg_t so that it sits next to operator<< (they need to match perfectly!) and have the coll_t one append _head or _TEMP as appropriate?

aiicore · 2015-11-20T08:48:43Z

@liewegas , I've moved calc_name into pg_t, but the buffer for the name sits in coll_t. I think it looks good except additional buf in operator<<(ostream& out, const pg_t &pg).
I've moved dec2char and hex2char outside class (temporary) - maybe those two could be used in more common way so I could move them to some global c/cc/h file. What do you think?
EDIT1: This change ignores TYPE_PG_REMOVAL, if it's really deprecated and not used I could throw away it from osd_types.cc/h or only from calc_str() -> there will be an assert().

loic-bot · 2015-11-20T13:52:49Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9447/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/9447/

liewegas · 2015-11-23T13:57:51Z

Can you squash the commits and repush so the buildbot runs again?

For the removal type, it is gone. I would remove it from everywhere.

aiicore · 2015-11-23T14:03:19Z

@liewegas , I've found bugs in my last commit and missing parts (spg_t). Probably tomorrow EOD I'll push fixed+squashed version.

loic-bot · 2015-11-25T08:28:02Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9541/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/9541/

aiicore · 2015-11-25T09:03:24Z

FAIL: test/encoding/readable.sh because ceph/ceph-object-corpus#2 should be merged first.

aclamk · 2015-12-03T09:07:18Z

@aiicore: Using those functions in separate C code (PoC) caused 11x reduction in cpu user time vs std::string and 2x vs sprintf() with the same pattern.
How did you measure this? Is there a ready-to use set of functions for this in Ceph?

aiicore · 2015-12-03T09:37:59Z

@aclamk So it's even better that is deprecated ;) I'll remove it. About testing, I used /usr/bin/time -v (not time - because it's bash built in) on my separate binary that contained only std::strings, operators << +, sprintf and my new functions in a loop. After merging to ceph I used my ceph_monitor (see my repo - it's very very early project) to see if cpu user time is dropping on whole cluster, and it was a little bit.

loic-bot · 2015-12-17T09:07:50Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/10264/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/10264/

aiicore · 2015-12-17T10:16:38Z

@dachary, The test is failing, because it's done on version checked out on commit from my pool request (I think so), and there are no changes from test/encoding/readable.sh and ceph-object-corpus yet.

Should I close and make new PR to trigger right version being build and tested? Or maybe there is some other way (from your side) to rebase my changes on top of current master and then do the test?

ghost · 2015-12-17T10:22:43Z

@aiicore you need to

git submodule update --remote ceph-object-corpus

and repush with the updated reference to the ceph-object-corpus.

loic-bot · 2015-12-17T11:13:33Z

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/10266/

SUCCESS http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/10266/

liewegas · 2015-12-17T18:06:21Z

src/osd/osd_types.cc

+    return 1;
+  }
+
+  while (i>=10) {


style nit: spaces around >=

liewegas · 2015-12-17T18:10:20Z

Can you please separate this into two patches: one that gets rid of the removal pg type (and changes the submodule), and the second that optimizes the calc_name code? Thanks!

aiicore · 2015-12-18T07:15:03Z

@liewegas Yes I could separate it on two PR's but removal pg type would be the first one and string reduce - second, thereby I don't need to care about bigger char _str buffer for FORREMOVAL... type

aiicore · 2015-12-18T13:07:31Z

@liewegas Drop removal pg type splited: #6970 after merge I'll fix this one.

liewegas · 2016-01-04T20:38:54Z

rebase?

efirs · 2016-01-05T23:50:16Z

@aiicore I think, functions can be greatly simplified and speed up, if you pass pointer to the end of the buffer and start serializing from the end.

efirs · 2016-01-06T00:12:11Z

@aiicore Something like this, but you don't need leading zeros: e4a8467

aiicore · 2016-01-07T07:14:10Z

@efirs I think you didn't even look what I've done in this PR :)
Look again:

I fill up buffer from the calculated "end".
The "end' position is calculated before, because I don't use leading zeros and I must know where the "end" is.
btw. Your function is very similar to mine. When my PR will be merged before yours, please make use of my *num_char_map = "0123456789abcdef"; instead of yours *buf-- = "0123456789"[i % 10]; - we are making changes in the same file. I'll comment your commit.

aiicore · 2016-01-07T07:21:49Z

@liewegas, forgot about style nit, pushed again.

Signed-off-by: Evgeniy Firsov <evgeniy.firsov@sandisk.com>

efirs · 2016-01-07T08:24:49Z

@aiicore

I fill up buffer from the calculated "end".

And that's exactly what you could avoid, saving cpu.

The "end' position is calculated before, because I don't use leading zeros and I must know where the "end" is.

Think, what if you start filling from the end in calc_name function, when you finish you'll have pointer to the beginning of what you just serialized.

BTW, I did look at what you've done in the PR. Good job. Just wanted to help make it a little bit better.

aiicore · 2016-01-07T09:49:59Z

@efirs Thanks!, I thought that you looked only on dec/hex2char functions. Ok, when you refer to whole PR filing from the end in calc_str() could be faster. I wrote dec/hex2char in a way, that they can be used more global.

I'll rewrite and check this locally, good tip!

aiicore · 2016-01-08T13:39:14Z

@efirs I think we'll have a compromise due function name ;) ritoa - like rindex (r = right). itoa - because arguments are just like in old/original itoa, so now it's one function to 10 and 16 base number. ritoa is also a template - to be consistent with your idea.

liewegas · 2016-01-08T14:04:25Z

src/osd/osd_types.h

@@ -523,7 +530,8 @@ class coll_t {
  spg_t pgid;
  uint64_t removal_seq;  // note: deprecated, not encoded

-  string _str;  // cached string
+  char _str_buff[spg_t::calc_name_buf_size];
+  char *_str = NULL;


This means we need an operator=. Otherwise we'll copy the pointer value and bad things will happen

I think I could save the _str_buff offset in a variable and use it in .to_str() and .c_str() and some other places like .parse(), so I could avoid operator=. I'll check on Monday those two options.

I don't think you need to initialize _str, calc_str() will always take care of that.
But even if you need to you can always initialize it in the constructor initialization list.
With _str code will look cleaner than with _str_offset.

@efirs Yep, calc_str() is called in constructors so no need for _str initialization. I'll revert to _str.

liewegas · 2016-01-08T14:06:19Z

The rest looks reasonable to me!

efirs · 2016-01-08T20:32:13Z

@aiicore LGTM, except _str_offset and base parameter I commented above.

1. Reduce string use. 2. Speedup dec and hex to char conversion. Signed-off-by: Igor Podoski <igor.podoski@ts.fujitsu.com>

osd: reduce string use in coll_t::calc_str() Reviewed-by: Sage Weil <sage@redhat.com> Reviewed-by: Evgeniy Firsov <evgeniy.firsov@sandisk.com>

liewegas added core performance labels Nov 9, 2015

aiicore force-pushed the reduce_strings branch from a4f8de3 to 82cf1e4 Compare November 10, 2015 07:53

liewegas self-assigned this Nov 23, 2015

aiicore mentioned this pull request Nov 25, 2015

Drop removal pg type. ceph/ceph-object-corpus#1

Closed

aiicore force-pushed the reduce_strings branch 3 times, most recently from e7b8392 to f64c60e Compare November 25, 2015 07:48

aiicore mentioned this pull request Nov 25, 2015

Drop deprecated removal pg type. ceph/ceph-object-corpus#2

Merged

aiicore force-pushed the reduce_strings branch from f64c60e to 52d5557 Compare December 17, 2015 07:55

aiicore force-pushed the reduce_strings branch from 52d5557 to cbfff92 Compare December 17, 2015 10:47

liewegas reviewed Dec 17, 2015
View reviewed changes

src/osd/osd_types.cc

return 1;

}

while (i>=10) {

Copy link

Member

liewegas Dec 17, 2015

Choose a reason for hiding this comment

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

style nit: spaces around >=

aiicore force-pushed the reduce_strings branch 2 times, most recently from 9d256aa to a0e5666 Compare January 5, 2016 14:17

aiicore force-pushed the reduce_strings branch from a0e5666 to 3951c32 Compare January 7, 2016 07:19

aiicore referenced this pull request Jan 7, 2016

osd: Replace snprintf with faster version in eversion_t::get_key_name

e4a8467

Signed-off-by: Evgeniy Firsov <evgeniy.firsov@sandisk.com>

aiicore force-pushed the reduce_strings branch 2 times, most recently from c0150f6 to fb34e91 Compare January 8, 2016 13:28

liewegas reviewed Jan 8, 2016
View reviewed changes

aiicore force-pushed the reduce_strings branch from fb34e91 to 444d8d5 Compare January 8, 2016 15:35

Reduce string use in coll_t::calc_str()

95e885e

1. Reduce string use. 2. Speedup dec and hex to char conversion. Signed-off-by: Igor Podoski <igor.podoski@ts.fujitsu.com>

aiicore force-pushed the reduce_strings branch from 444d8d5 to 95e885e Compare January 11, 2016 12:18

liewegas added needs-qa wip-sage2-testing labels Jan 11, 2016

liewegas added a commit that referenced this pull request Jan 13, 2016

Merge pull request #6505 from aiicore/reduce_strings

92d3d17

osd: reduce string use in coll_t::calc_str() Reviewed-by: Sage Weil <sage@redhat.com> Reviewed-by: Evgeniy Firsov <evgeniy.firsov@sandisk.com>

liewegas merged commit 92d3d17 into ceph:master Jan 13, 2016

aiicore deleted the reduce_strings branch January 13, 2016 07:00

ghost changed the title ~~Reduce string use in coll_t::calc_str()~~ osd: reduce string use in coll_t::calc_str() Feb 10, 2016

aiicore mentioned this pull request Nov 10, 2016

osd: Replace snprintf with faster implementation in eversion_t::get_key_name #7121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: reduce string use in coll_t::calc_str() #6505

osd: reduce string use in coll_t::calc_str() #6505

aiicore commented Nov 9, 2015

liewegas commented Nov 9, 2015

aiicore commented Nov 20, 2015

loic-bot commented Nov 20, 2015

liewegas commented Nov 23, 2015

aiicore commented Nov 23, 2015

loic-bot commented Nov 25, 2015

aiicore commented Nov 25, 2015

aclamk commented Dec 3, 2015

aiicore commented Dec 3, 2015

loic-bot commented Dec 17, 2015

aiicore commented Dec 17, 2015

ghost commented Dec 17, 2015

loic-bot commented Dec 17, 2015

liewegas Dec 17, 2015

liewegas commented Dec 17, 2015

aiicore commented Dec 18, 2015

aiicore commented Dec 18, 2015

liewegas commented Jan 4, 2016

efirs commented Jan 5, 2016

efirs commented Jan 6, 2016

aiicore commented Jan 7, 2016

aiicore commented Jan 7, 2016

efirs commented Jan 7, 2016

aiicore commented Jan 7, 2016

aiicore commented Jan 8, 2016

liewegas Jan 8, 2016

aiicore Jan 8, 2016

efirs Jan 8, 2016

aiicore Jan 11, 2016

liewegas commented Jan 8, 2016

efirs commented Jan 8, 2016

osd: reduce string use in coll_t::calc_str() #6505

osd: reduce string use in coll_t::calc_str() #6505

Conversation

aiicore commented Nov 9, 2015

liewegas commented Nov 9, 2015

aiicore commented Nov 20, 2015

loic-bot commented Nov 20, 2015

liewegas commented Nov 23, 2015

aiicore commented Nov 23, 2015

loic-bot commented Nov 25, 2015

aiicore commented Nov 25, 2015

aclamk commented Dec 3, 2015

aiicore commented Dec 3, 2015

loic-bot commented Dec 17, 2015

aiicore commented Dec 17, 2015

ghost commented Dec 17, 2015

loic-bot commented Dec 17, 2015

liewegas Dec 17, 2015

Choose a reason for hiding this comment

liewegas commented Dec 17, 2015

aiicore commented Dec 18, 2015

aiicore commented Dec 18, 2015

liewegas commented Jan 4, 2016

efirs commented Jan 5, 2016

efirs commented Jan 6, 2016

aiicore commented Jan 7, 2016

aiicore commented Jan 7, 2016

efirs commented Jan 7, 2016

aiicore commented Jan 7, 2016

aiicore commented Jan 8, 2016

liewegas Jan 8, 2016

Choose a reason for hiding this comment

aiicore Jan 8, 2016

Choose a reason for hiding this comment

efirs Jan 8, 2016

Choose a reason for hiding this comment

aiicore Jan 11, 2016

Choose a reason for hiding this comment

liewegas commented Jan 8, 2016

efirs commented Jan 8, 2016