>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 <cbodley@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- 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
