Some more findings.. 1. By using make_shared the cost of shared_ptr is reduced significantly..
##### Test conventional ptr ###### micros_used for conventional ptr: 22633702 ##### Test Unique Smart ptr ###### micros_used for unique ptr: 22633694 ##### Test Shared Smart ptr ###### micros_used for shared ptr: 27105354 So, almost half now... Will do some cleanup in the ceph io path on this.. 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. 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.. Thanks & Regards Somnath -----Original Message----- From: Samuel Just [mailto:[email protected]] Sent: Thursday, December 03, 2015 3:24 PM To: Casey Bodley Cc: Sage Weil; Somnath Roy; Samuel Just ([email protected]); [email protected] Subject: Re: queue_transaction interface + unique_ptr + performance From a simplicity point of view, I'd rather just move a Transaction object than use a unique_ptr. Maybe the overhead doesn't end up being significant? -Sam On Thu, Dec 3, 2015 at 1:23 PM, Casey Bodley <[email protected]> wrote: > > ----- Original Message ----- >> On Thu, 3 Dec 2015, Casey Bodley wrote: >> > > Well, yeah we are, it's just the actual Transaction structure >> > > which wouldn't be dynamic -- the buffers and many other fields >> > > would still hit the allocator. >> > > -Sam >> > >> > Sure. I was looking specifically at the tradeoffs between >> > allocating and moving the Transaction object itself. >> > >> > As it currently stands, the caller of ObjectStore can choose >> > whether to allocate its Transactions on the heap, embed them in >> > other objects, or put them on the stack for use with >> > apply_transactions(). Switching to an interface built around >> > unique_ptr forces all callers to use the heap. I'm advocating for an >> > interface that doesn't. >> >> That leaves us with either std::move or.. the raw Transaction* we >> have now. Right? > > Right. The thing I really like about the unique_ptr approach is that > the caller no longer has to care about the Transaction's lifetime, so > doesn't have to allocate the extra ObjectStore::C_DeleteTransaction for > cleanup. > Movable Transactions accomplish this as well. > > Casey > >> >> > > > It's true that the move ctor has to do work. I counted 18 >> > > > fields, half of which are integers, and the rest have move >> > > > ctors themselves. But the cpu is good at integers. The win here >> > > > is that you're not hitting the allocator in the fast path. >> >> To be fair, many of these are also legacy that we can remove... >> possibly even now. IIRC the only exposure to legacy encoded >> transactions (that use the tbl hackery) are journal items from an >> upgrade pre-hammer OSD that aren't flushed on upgrade. We should >> have made the osd flush the journal before recording the 0_94_4 >> ondisk feature. We could add another one to enforce that and rip all >> that code out now instead of waiting until after jewel... that would >> be satisfying (and I think an ondisk ceph-osd feature is enough here, >> then document that users should upgrade to hammer 0.94.6 or infernalis 9.2.1 >> before moving to jewel). >> >> sage >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" >> in the body of a message to [email protected] More majordomo >> info at http://vger.kernel.org/majordomo-info.html >>
