Hi, Please review the following pull request addressing this.. https://github.com/ceph/ceph/pull/7271 It is working well in my tests with 384 TB cluster, 48 OSDs , 6 OSD nodes..It is giving ~3-4% cpu usage improvement as well.. Thanks & Regards Somnath -----Original Message----- From: Somnath Roy Sent: Thursday, December 03, 2015 10:44 PM To: 'Adam C. Emerson' Cc: 'Samuel Just'; 'Casey Bodley'; 'Sage Weil'; 'Samuel Just (sam.just@xxxxxxxxxxx)'; 'ceph-devel@xxxxxxxxxxxxxxx' Subject: RE: queue_transaction interface + unique_ptr + performance Sorry , I went through entire conversation and realized that we are expecting the move constructor of Transaction class won't be inducing much overhead vs unique_ptr. Thanks I will start coding around that and will publish the final interfaces soon for the sign off. Regards Somnath -----Original Message----- From: Somnath Roy Sent: Thursday, December 03, 2015 10:16 PM To: 'Adam C. Emerson' Cc: Samuel Just; Casey Bodley; Sage Weil; Samuel Just (sam.just@xxxxxxxxxxx); ceph-devel@xxxxxxxxxxxxxxx Subject: RE: queue_transaction interface + unique_ptr + performance Adam, May be I am missing something here. Don't we need to do an extra Transaction copy in case of async completion and Transaction is allocated on stack for queue_transaction() taking vector<Transaction> ? Thanks & Regards Somnath -----Original Message----- From: Adam C. Emerson [mailto:aemerson@xxxxxxxxxx] Sent: Thursday, December 03, 2015 5:44 PM To: Somnath Roy Cc: Samuel Just; Casey Bodley; Sage Weil; Samuel Just (sam.just@xxxxxxxxxxx); ceph-devel@xxxxxxxxxxxxxxx Subject: Re: queue_transaction interface + unique_ptr + performance On 04/12/2015, Somnath Roy wrote: [snip] > ##### Test Shared Smart ptr ###### > micros_used for shared ptr: 27105354 One thing to keep in mind is that, if there are any cases where we really DO want to use shared_ptr, if we find ourselves assigning a new pointer then releasing an old one, it's better to std::move from one to the other, since it doesn't have to use locking/atomics to fiddle with the refcount in that case. But if we can avoid them, all the better. [snip] > 2. I have also measured the cost of list::push_back etc. , all is expected.. if I replace list with vector again I am getting *some benefit* , which is also expected I guess. If we have some idea how many elements we expect/want to hold, calling reserve will get all our allocations done all at once rather than dipping into the heap multiple times. > I am not seeing any conclusion on the interface change...Here is my understanding so far.. > > 1. For the method like apply_transaction() which is taking Transaction reference now, I will not be changing the interface , instead will do an extra copy to create unique_ptr inside. > > 2. For all the cases taking Transaction* , I will change that to > TransactionRef&& > > 3. The queue_transactions() will be changed to vector< TransactionRef> > instead of list< Transaction*> > > Let me know if I am missing anything.. My understanding is that in some cases we might not want to use the heap. (For example, if we're executing a transaction synchronously rather than enqueing it.) And to make Transaction movable. Then we would not build a std::vector<TransactionRef>, what we would build instead is std::vector<Transaction> In this case, we should probably have everything just take Transaction&& (so the caller can expect to have the contents of the transaction moved out. We might also want to consider building a transaction in place. I know I saw one constructor that takes bufferlist. If there are other cases where we could benefit from just passing in arguments and having the transaction built right into the vector (with emplace_back), that would be even better than constructing it on the stack and moving it in to the vector. -- Senior Software Engineer Red Hat Storage, Ann Arbor, MI, US IRC: Aemerson@{RedHat, OFTC, Freenode} 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C 7C12 80F7 544B 90ED BFB9 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
