osd: eliminate ineffective container operations #19099

ifed01 · 2017-11-22T13:29:01Z

Signed-off-by: Igor Fedotov ifedotov@suse.com

gregsfortytwo · 2017-11-22T15:43:30Z

What about this is "more effective" at manipulating pglog entries? With the switch from eversion_t to string, it just looks slower.

ifed01 · 2017-11-23T12:06:58Z

@gregsfortytwo This is primarily about more effective (no copy) moving trimmed/trimmed_dups containers content to to_remove one in _write_log_and_missing... methods. But you're right that tracking map of strings is less effective than map of eversion_t. Reverted this part. Thanks.

tchaikov · 2017-11-23T13:23:19Z

src/osd/PG.cc

-      peer_info[peer].last_epoch_started = info.last_epoch_started;
-      peer_info[peer].last_interval_started = info.last_interval_started;
-      peer_info[peer].history.merge(info.history);
+    auto it = peer_info.find(peer);


i'd suggest just s/it/peer/ . so

auto peer = peer_info.find(*i); ...

easier to read this way, IMHO.

ifed01 · 2017-11-23T14:39:41Z

@tchaikov - updated

tchaikov · 2017-11-28T10:43:06Z

src/os/Transaction.cc

@@ -337,6 +337,12 @@ void ObjectStore::Transaction::dump(ceph::Formatter *f)
 	f->dump_string("op_name", "omap_rmkeys");
 	f->dump_stream("collection") << cid;
 	f->dump_stream("oid") << oid;
+	f->open_object_section("attrs");


might want to use open_array_section() instead.

tchaikov · 2017-11-28T10:43:14Z

src/os/Transaction.cc

@@ -337,6 +337,12 @@ void ObjectStore::Transaction::dump(ceph::Formatter *f)
 	f->dump_string("op_name", "omap_rmkeys");
 	f->dump_stream("collection") << cid;
 	f->dump_stream("oid") << oid;
+	f->open_object_section("attrs");
+	for (auto p = keys.begin();


could use range-based loop.

gregsfortytwo · 2017-11-29T00:18:57Z

I'll defer to Kefu on a proper review, but I don't have other objections after a skim. :)

ifed01 · 2017-11-29T11:23:50Z

@tchaikov - updated

tchaikov · 2017-12-01T09:46:54Z

src/osd/PGLog.cc

@@ -621,8 +621,8 @@ void PGLog::write_log_and_missing_wo_missing(
  _write_log_and_missing_wo_missing(
    t, km, log, coll, log_oid,
    divergent_priors, eversion_t::max(), eversion_t(), eversion_t(),
-    set<eversion_t>(),
-    set<string>(),
+    nullptr,


the only caller of PGLog::_write_log_and_missing_wo_missing() is PGLog::write_log_and_missing_wo_missing(). and the latter always pass it empty trimmed and trimmed_dups. can we take this chance to remove them and related logic as well. as they are not used at all.

tchaikov · 2017-12-01T09:47:49Z

src/osd/PGLog.cc

-  for (set<eversion_t>::const_iterator i = trimmed.begin();
-       i != trimmed.end();
+  set<string> to_remove;
+  to_remove.swap(*trimmed_dups);


dereferencing a nullptr, will crash the application. and we always pass nullptrs to this function.
see c2e2f03#diff-2bf6ec52b400c95e6a23a186482477b6R621

tchaikov · 2017-12-01T09:49:51Z

src/osd/PGLog.cc

@@ -793,8 +797,8 @@ void PGLog::_write_log_and_missing(
  eversion_t dirty_to,
  eversion_t dirty_from,
  eversion_t writeout_from,
-  const set<eversion_t> &trimmed,
-  const set<string> &trimmed_dups,
+  set<eversion_t> *trimmed,


if my argument holds, we can remove these two parameters. and remove the following code you are changing in this PR.

this is not addressed.

@tchaikov - your arguments are valid for _write_log_and_missing_wo_missing - there is just a single call of it. But for _write_log_and_missing trimmed/trimmed_dups are still applicable and are used when pg log trimming occurs. Hope I haven't missed something...

@tchaikov - ping!

@ifed01 sorry for the latency, will review it early tomorrow.

ifed01 · 2017-12-01T19:20:35Z

@tchaikov - thanks, updated.

liewegas · 2017-12-04T15:44:29Z

@tchaikov ping

tchaikov · 2017-12-13T11:19:03Z

src/osd/PGLog.cc

-      log_keys_debug->erase(i->get_key_name());
+  set<string> to_remove;
+  if (trimmed_dups) {
+    to_remove.swap(*trimmed_dups);


this changes the semantic of this method. before this change, trimmed and trimmed_dups are not changed after _write_log_and_missing() returns. after this change, both of them are empty. is this expected?

yes, that's expected. they aren't used any more

if that's the case, i need to take a closer look.

and i'd suggest update the commit message accordingly.

@tchaikov - updated

nit, could just

to_remove = std::move(*trimmed_dups);

as we don't care about trimmed_dups anymore after this call.

IMO it's better to leave a container in a consistent(=empty) state to avoid potential issues if code evolves.

tchaikov · 2017-12-13T11:19:37Z

src/osd/PGLog.cc

+    to_remove.swap(*trimmed_dups);
+  }
+  if (trimmed) {
+    for (auto i = trimmed->begin();


nit, might want to take this chance to use range-based loop.

tchaikov

aside from some nits, lgtm. and I don't feel strong about them.

tchaikov · 2018-01-03T09:31:22Z

src/osd/PGLog.cc

-      log_keys_debug->erase(i->get_key_name());
+  set<string> to_remove;
+  if (trimmed_dups) {
+    to_remove.swap(*trimmed_dups);


nit, could just

to_remove = std::move(*trimmed_dups);

as we don't care about trimmed_dups anymore after this call.

tchaikov · 2018-01-03T09:31:45Z

src/osd/PGLog.cc

    }
+    trimmed->clear();


same here, this call is not needed.

tchaikov · 2018-01-03T09:36:29Z

src/osd/PGLog.h

@@ -1239,8 +1237,8 @@ struct PGLog : DoutPrefixProvider {
    eversion_t dirty_to,
    eversion_t dirty_from,
    eversion_t writeout_from,
-    const set<eversion_t> &trimmed,
-    const set<string> &trimmed_dups,
+    set<eversion_t> *trimmed,


could pass by rvalue reference, which reflects the intention of this function better. the up side of using pointer is that it saves the overhead of constructing a temporary empty set<>, but the only case we pass nullptr to _write_log_and_missing() is in ceph_objectstore_tool.cc, so I don't think that's a bottleneck we need to worry about.

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

histogram Signed-off-by: Igor Fedotov <ifedotov@suse.com>

…ntics whenever possible Signed-off-by: Igor Fedotov <ifedotov@suse.com>

…al_repop. Signed-off-by: Igor Fedotov <ifedotov@suse.com>

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

liewegas · 2018-01-11T21:50:35Z

http://pulpito.ceph.com/sage-2018-01-11_15:01:24-rados-wip-sage3-testing-2018-01-11-0711-distro-basic-smithi/#

ifed01 added the performance label Nov 22, 2017

ifed01 force-pushed the wip-ifed-faster-pglog branch from 5cce967 to c00944c Compare November 23, 2017 11:54

ifed01 changed the title ~~os/PGLog: manipulate trimmed entries more effectively~~ osd: eliminate ineffective container operations Nov 23, 2017

tchaikov reviewed Nov 23, 2017

View reviewed changes

ifed01 force-pushed the wip-ifed-faster-pglog branch from 89db4a0 to 010f247 Compare November 23, 2017 14:04

tchaikov self-requested a review November 24, 2017 16:33

tchaikov reviewed Nov 28, 2017

View reviewed changes

ifed01 force-pushed the wip-ifed-faster-pglog branch from 010f247 to 22d7d0e Compare November 29, 2017 11:20

tchaikov self-requested a review November 29, 2017 11:54

tchaikov requested changes Dec 1, 2017

View reviewed changes

ifed01 force-pushed the wip-ifed-faster-pglog branch from 22d7d0e to bb003d2 Compare December 1, 2017 19:19

ifed01 force-pushed the wip-ifed-faster-pglog branch 2 times, most recently from fb25f68 to 91008f7 Compare December 12, 2017 12:05

tchaikov self-requested a review December 12, 2017 14:42

tchaikov reviewed Dec 13, 2017

View reviewed changes

ifed01 force-pushed the wip-ifed-faster-pglog branch from 91008f7 to bd86091 Compare December 14, 2017 13:04

tchaikov self-requested a review December 14, 2017 14:08

tchaikov approved these changes Jan 3, 2018

View reviewed changes

tchaikov added the needs-qa label Jan 3, 2018

ifed01 added 4 commits January 3, 2018 14:45

os/Transaction: print keys when dumping omap_removekeys transaction

01cda74

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

os/bluestore: distinguish regular and pgmeta omaps when generating DB

b40131a

histogram Signed-off-by: Igor Fedotov <ifedotov@suse.com>

osd/PGLog: refactor 'trimmed' container processing to use 'move' sema…

6df13e6

…ntics whenever possible Signed-off-by: Igor Fedotov <ifedotov@suse.com>

osd/PrimryLogPG: avoid repetitive lookups on waiting_for_ondisk in ev…

3f11a47

…al_repop. Signed-off-by: Igor Fedotov <ifedotov@suse.com>

ifed01 added 2 commits January 3, 2018 16:18

osd/pglog: avoid excessive lookups in unindex method

3ad6456

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

osd/PG: avoid excessive lookups in share_pg_info()

dfcdf7c

Signed-off-by: Igor Fedotov <ifedotov@suse.com>

ifed01 force-pushed the wip-ifed-faster-pglog branch from bd86091 to dfcdf7c Compare January 3, 2018 13:24

liewegas added the wip-sage3-testing label Jan 10, 2018

liewegas merged commit c6d6676 into ceph:master Jan 11, 2018

ifed01 deleted the wip-ifed-faster-pglog branch January 16, 2018 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: eliminate ineffective container operations #19099

osd: eliminate ineffective container operations #19099

ifed01 commented Nov 22, 2017

gregsfortytwo commented Nov 22, 2017

ifed01 commented Nov 23, 2017

tchaikov Nov 23, 2017

ifed01 commented Nov 23, 2017

tchaikov Nov 28, 2017

tchaikov Nov 28, 2017

gregsfortytwo commented Nov 29, 2017

ifed01 commented Nov 29, 2017

tchaikov Dec 1, 2017

tchaikov Dec 1, 2017

tchaikov Dec 1, 2017

tchaikov Dec 4, 2017

ifed01 Dec 4, 2017

ifed01 Dec 12, 2017

tchaikov Dec 12, 2017

ifed01 commented Dec 1, 2017

liewegas commented Dec 4, 2017

tchaikov Dec 13, 2017

ifed01 Dec 13, 2017

tchaikov Dec 13, 2017

tchaikov Dec 13, 2017

ifed01 Dec 14, 2017

tchaikov Jan 3, 2018

ifed01 Jan 3, 2018

tchaikov Dec 13, 2017

tchaikov left a comment

tchaikov Jan 3, 2018

tchaikov Jan 3, 2018

tchaikov Jan 3, 2018

ifed01 Jan 3, 2018

liewegas commented Jan 11, 2018

osd: eliminate ineffective container operations #19099

osd: eliminate ineffective container operations #19099

Conversation

ifed01 commented Nov 22, 2017

gregsfortytwo commented Nov 22, 2017

ifed01 commented Nov 23, 2017

Choose a reason for hiding this comment

ifed01 commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsfortytwo commented Nov 29, 2017

ifed01 commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifed01 commented Dec 1, 2017

liewegas commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Jan 11, 2018