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.

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

os/filestore: avoid unnecessary copy in filestore::_do_transaction #12578

Merged
merged 4 commits into from Mar 13, 2017

Conversation

Projects
None yet
5 participants
@wwformat
Copy link
Contributor

wwformat commented Dec 20, 2016

No description provided.

Yunchuan Wen added some commits Dec 20, 2016

Yunchuan Wen
filestore: avoid unnecessary copy with ghobject_t
Signed-off-by: Yunchuan Wen <yunchuan.wen@kylin-cloud.com>
Yunchuan Wen
filestore: avoid unnecessary copy with coll_t
Signed-off-by: Yunchuan Wen <yunchuan.wen@kylin-cloud.com>
Yunchuan Wen
filestore: remove unused routine
_kludge_temp_object_collection is unused routine now.

Signed-off-by: Yunchuan Wen <yunchuan.wen@kylin-cloud.com>
@Liuchang0812
Copy link
Contributor

Liuchang0812 left a comment

hi, thanks for your working.

It's just my thought whether we can use c++11's forward here. std::forward and std::move is clearer and have a good semantics.

Yunchuan Wen
filestore: avoid copy ghobject_t in _check_replay_guard
Signed-off-by: Yunchuan Wen <yunchuan.wen@kylin-cloud.com>
const ghobject_t &oid = i.get_oid(op->oid);
_kludge_temp_object_collection(cid, oid);
const coll_t &cid = !_need_temp_object_collection(_cid, oid) ?
_cid : _cid.get_temp();

This comment has been minimized.

Copy link
@liewegas

liewegas Dec 20, 2016

Member

does this work to assign a const coll_t& ref to a temporary return value?

This comment has been minimized.

Copy link
@wwformat

wwformat Dec 20, 2016

Author Contributor

@liewegas
Yes, I think const X& can reference a temporary object, and X& cannot.
And I found a similar change in commit 18fe93c, like this:

@@ -2882,10 +2882,10 @@ void FileStore::_do_transaction(
// --------------------
// objects

-bool FileStore::exists(coll_t cid, const ghobject_t& oid)
+bool FileStore::exists(const coll_t& _cid, const ghobject_t& oid)
{
- tracepoint(objectstore, exists_enter, cid.c_str());
- _kludge_temp_object_collection(cid, oid);
+ tracepoint(objectstore, exists_enter, _cid.c_str());
+ const coll_t& cid = !_need_temp_object_collection(_cid, oid) ? _cid : _cid.get_temp();
struct stat st;
bool retval = stat(cid, oid, &st) == 0;
tracepoint(objectstore, exists_exit, retval);

This comment has been minimized.

@wwformat

This comment has been minimized.

Copy link
Contributor Author

wwformat commented Dec 28, 2016

@liewegas ping

@liewegas liewegas changed the title filestore: avoid unnecessary copy in filestore::_do_transaction os/filestore: avoid unnecessary copy in filestore::_do_transaction Dec 28, 2016

@tchaikov tchaikov added the needs-qa label Mar 1, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Mar 3, 2017

@liewegas i think this change makes sense to me albeit more verbose than before due to the limitation of C++ syntax.

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Mar 3, 2017

Yeah; let's test! I'm a little nervous just because it's many opportunities to introduce a bug via a typo. @wwformat is there a measureable performance impact?

@liewegas liewegas merged commit abce6de into ceph:master Mar 13, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.