seastar temporary_buffer/packet and ceph::buffer

seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

hey Casey,


i've been thinking about how to better integrate
seastar::temporary_buffer and seastar::packet into our denc and
messenger subsystems based on your wip-seastar-msg branch.

as you pointed out to me that

> the conversions between ceph::buffer::ptr and seastar::temporary_buffer in https://github.com/cbodley/ceph/tree/wip-seastar-msg/ work, but they cost an allocation in both directions (seastar::make_object_deleter() has to allocate for type erasure).

and just like ceph::buffer::ptr, seastar::temporary_buffer also
manages the life cycle of the underlying memory chunk using refcount.
it's a waste to do the refcounting for the same mem chunk twice in the
same application.

if we want to avoid the overhead of duplicated refcounting and
allocation in the conversion to and from ceph::buffer::ptr, i think,
we need to allocate the temporary_buffer along with buffer::ptr. in
other words, to include seastar::temporary_buffer as a member variable
in ceph::buffer:ptr.

i am preparing a change to prototype this approach. the downside of it is:

- we need to include a placeholder structure in buffer.h for
seastar::tempoary_buffer<char>. and cast it to
seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
include seastar headers in librados.

cheers,


-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

On Mon, Jan 15, 2018 at 11:49 PM, kefu chai <tchaikov@gmail.com> wrote:
> hey Casey,
>
>
> i've been thinking about how to better integrate
> seastar::temporary_buffer and seastar::packet into our denc and
> messenger subsystems based on your wip-seastar-msg branch.
>
> as you pointed out to me that
>
>> the conversions between ceph::buffer::ptr and seastar::temporary_buffer in https://github.com/cbodley/ceph/tree/wip-seastar-msg/ work, but they cost an allocation in both directions (seastar::make_object_deleter() has to allocate for type erasure).
>
> and just like ceph::buffer::ptr, seastar::temporary_buffer also
> manages the life cycle of the underlying memory chunk using refcount.
> it's a waste to do the refcounting for the same mem chunk twice in the
> same application.
>
> if we want to avoid the overhead of duplicated refcounting and
> allocation in the conversion to and from ceph::buffer::ptr, i think,
> we need to allocate the temporary_buffer along with buffer::ptr. in
> other words, to include seastar::temporary_buffer as a member variable
> in ceph::buffer:ptr.
>
> i am preparing a change to prototype this approach. the downside of it is:
>
> - we need to include a placeholder structure in buffer.h for
> seastar::tempoary_buffer<char>. and cast it to
> seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
> include seastar headers in librados.

- we cannot store the crc map along with the raw class anymore. this
might be a show stopper.
- we can not use zero copy interfaces anymore. but i think it's fine.
as we need to use future/promise based interfaces for IO.

>
> cheers,
>
>
> --
> Regards
> Kefu Chai



-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[Piotr Dałek]

On Tue, Jan 16, 2018 at 12:53:22AM +0800, kefu chai wrote:
> On Mon, Jan 15, 2018 at 11:49 PM, kefu chai <tchaikov@gmail.com> wrote:
> > [..]
> > i am preparing a change to prototype this approach. the downside of it is:
> >
> > - we need to include a placeholder structure in buffer.h for
> > seastar::tempoary_buffer<char>. and cast it to
> > seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
> > include seastar headers in librados.
> 
> - we cannot store the crc map along with the raw class anymore. this
> might be a show stopper.

https://github.com/ceph/ceph/pull/18906
There, already fixed it.

-- 
Piotr Dałek
branch@predictor.org.pl
http://blog.predictor.org.pl

Re: seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

On Tue, Jan 16, 2018 at 1:04 AM, Piotr Dałek <branch@predictor.org.pl> wrote:
> On Tue, Jan 16, 2018 at 12:53:22AM +0800, kefu chai wrote:
>> On Mon, Jan 15, 2018 at 11:49 PM, kefu chai <tchaikov@gmail.com> wrote:
>> > [..]
>> > i am preparing a change to prototype this approach. the downside of it is:
>> >
>> > - we need to include a placeholder structure in buffer.h for
>> > seastar::tempoary_buffer<char>. and cast it to
>> > seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
>> > include seastar headers in librados.
>>
>> - we cannot store the crc map along with the raw class anymore. this
>> might be a show stopper.
>
> https://github.com/ceph/ceph/pull/18906
> There, already fixed it.


Piotr, thanks for point me to this PR. but i am evaluating the pros
and cons of the change, which tries to replace buffer::raw with
seastar::temporary_buffer, and use the latter in buffer::ptr as a
member variable.

>
> --
> Piotr Dałek
> branch@predictor.org.pl
> http://blog.predictor.org.pl



-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[Piotr Dałek]

On Tue, Jan 16, 2018 at 01:09:57AM +0800, kefu chai wrote:
> On Tue, Jan 16, 2018 at 1:04 AM, Piotr Dałek <branch@predictor.org.pl> wrote:
> > On Tue, Jan 16, 2018 at 12:53:22AM +0800, kefu chai wrote:
> >> On Mon, Jan 15, 2018 at 11:49 PM, kefu chai <tchaikov@gmail.com> wrote:
> >> > [..]
> >> > i am preparing a change to prototype this approach. the downside of it is:
> >> >
> >> > - we need to include a placeholder structure in buffer.h for
> >> > seastar::tempoary_buffer<char>. and cast it to
> >> > seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
> >> > include seastar headers in librados.
> >>
> >> - we cannot store the crc map along with the raw class anymore. this
> >> might be a show stopper.
> >
> > https://github.com/ceph/ceph/pull/18906
> > There, already fixed it.
> 
> Piotr, thanks for point me to this PR. but i am evaluating the pros
> and cons of the change, which tries to replace buffer::raw with
> seastar::temporary_buffer, and use the latter in buffer::ptr as a
> member variable.

And I'm pointing out that there's no need to worry about crc map anymore,
because it's gone.

-- 
Piotr Dałek
branch@predictor.org.pl
http://blog.predictor.org.pl

Re: seastar temporary_buffer/packet and ceph::buffer

[Casey Bodley]

Hi Kefu, thanks for your feedback on this!

To add some context for the list, I had commented on Kefu's denc pr at 
https://github.com/ceph/ceph/pull/19662#issuecomment-355602713 about 
integrating ceph encode/decode with buffer types from other frameworks, 
using the seastar types as an example (seastar::temporary_buffer is a 
buffer segment with ownership semantics, so shares a lot in common with 
ceph::bufferptr).

My approach in the seastar messenger added buffer conversions either by 
wrapping the seastar::temporary_buffer inside of a raw buffer type, or 
by wrapping our ceph::bufferptr inside a seastar::temporary_buffer's 
deleter object. So the conversions in both directions cost an allocation.

On 01/15/2018 10:49 AM, kefu chai wrote:
> hey Casey,
>
>
> i've been thinking about how to better integrate
> seastar::temporary_buffer and seastar::packet into our denc and
> messenger subsystems based on your wip-seastar-msg branch.
>
> as you pointed out to me that
>
>> the conversions between ceph::buffer::ptr and seastar::temporary_buffer in https://github.com/cbodley/ceph/tree/wip-seastar-msg/ work, but they cost an allocation in both directions (seastar::make_object_deleter() has to allocate for type erasure).
> and just like ceph::buffer::ptr, seastar::temporary_buffer also
> manages the life cycle of the underlying memory chunk using refcount.
> it's a waste to do the refcounting for the same mem chunk twice in the
> same application.
>
> if we want to avoid the overhead of duplicated refcounting and
> allocation in the conversion to and from ceph::buffer::ptr, i think,
> we need to allocate the temporary_buffer along with buffer::ptr. in
> other words, to include seastar::temporary_buffer as a member variable
> in ceph::buffer:ptr.
>
> i am preparing a change to prototype this approach. the downside of it is:
>
> - we need to include a placeholder structure in buffer.h for
> seastar::tempoary_buffer<char>. and cast it to
> seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
> include seastar headers in librados.
>
> cheers,
>
>

Interesting, I hadn't thought about adding it as a member of bufferptr. 
That would be add some size overhead for non-seastar buffers though, 
unless we were able to reuse that for all raw buffer types. I agree that 
we want to be careful about dependencies, though, so I was really hoping 
to avoid this kind of coupling. I'd really prefer not to have any of our 
common libraries depend on seastar, and only link it directly to the 
targets that need it.

My initial idea was that, if we were able to refactor denc in terms of 
some generic buffer concept, we'd be able to use ceph's encode/decode on 
external buffer types without the need to convert to/from bufferlist in 
the first place. That would allow us to support seastar's buffer types, 
and also extend support to other types like boost::asio's buffer 
concepts as well.

So my comment on your pr was asking more about specific features that 
denc requires from bufferlist, so that we could start to generalize them 
in a way that doesn't have bufferlist baked in.

Thanks,
Casey

Re: seastar temporary_buffer/packet and ceph::buffer

[Matt Benjamin]

It's the buffer::raw class that abstracts foreign buffer
representations now, is it now? A mechanism to allow seastar to
single-allocate a composite buffer::raw and temporary_buffer<char>
seems possible to imagine.

Matt

On Mon, Jan 15, 2018 at 12:19 PM, Casey Bodley <cbodley@redhat.com> wrote:
> Hi Kefu, thanks for your feedback on this!
>
> To add some context for the list, I had commented on Kefu's denc pr at
> https://github.com/ceph/ceph/pull/19662#issuecomment-355602713 about
> integrating ceph encode/decode with buffer types from other frameworks,
> using the seastar types as an example (seastar::temporary_buffer is a buffer
> segment with ownership semantics, so shares a lot in common with
> ceph::bufferptr).
>
> My approach in the seastar messenger added buffer conversions either by
> wrapping the seastar::temporary_buffer inside of a raw buffer type, or by
> wrapping our ceph::bufferptr inside a seastar::temporary_buffer's deleter
> object. So the conversions in both directions cost an allocation.
>
> On 01/15/2018 10:49 AM, kefu chai wrote:
>>
>> hey Casey,
>>
>>
>> i've been thinking about how to better integrate
>> seastar::temporary_buffer and seastar::packet into our denc and
>> messenger subsystems based on your wip-seastar-msg branch.
>>
>> as you pointed out to me that
>>
>>> the conversions between ceph::buffer::ptr and seastar::temporary_buffer
>>> in https://github.com/cbodley/ceph/tree/wip-seastar-msg/ work, but they cost
>>> an allocation in both directions (seastar::make_object_deleter() has to
>>> allocate for type erasure).
>>
>> and just like ceph::buffer::ptr, seastar::temporary_buffer also
>> manages the life cycle of the underlying memory chunk using refcount.
>> it's a waste to do the refcounting for the same mem chunk twice in the
>> same application.
>>
>> if we want to avoid the overhead of duplicated refcounting and
>> allocation in the conversion to and from ceph::buffer::ptr, i think,
>> we need to allocate the temporary_buffer along with buffer::ptr. in
>> other words, to include seastar::temporary_buffer as a member variable
>> in ceph::buffer:ptr.
>>
>> i am preparing a change to prototype this approach. the downside of it is:
>>
>> - we need to include a placeholder structure in buffer.h for
>> seastar::tempoary_buffer<char>. and cast it to
>> seastar::tempoary_buffer<char> in buffer.cc . otherwise we need to
>> include seastar headers in librados.
>>
>> cheers,
>>
>>
>
> Interesting, I hadn't thought about adding it as a member of bufferptr. That
> would be add some size overhead for non-seastar buffers though, unless we
> were able to reuse that for all raw buffer types. I agree that we want to be
> careful about dependencies, though, so I was really hoping to avoid this
> kind of coupling. I'd really prefer not to have any of our common libraries
> depend on seastar, and only link it directly to the targets that need it.
>
> My initial idea was that, if we were able to refactor denc in terms of some
> generic buffer concept, we'd be able to use ceph's encode/decode on external
> buffer types without the need to convert to/from bufferlist in the first
> place. That would allow us to support seastar's buffer types, and also
> extend support to other types like boost::asio's buffer concepts as well.
>
> So my comment on your pr was asking more about specific features that denc
> requires from bufferlist, so that we could start to generalize them in a way
> that doesn't have bufferlist baked in.
>
> Thanks,
> Casey
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 

Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

Re: seastar temporary_buffer/packet and ceph::buffer

[Casey Bodley]

On 01/15/2018 02:30 PM, Matt Benjamin wrote:
> It's the buffer::raw class that abstracts foreign buffer
> representations now, is it now? A mechanism to allow seastar to
> single-allocate a composite buffer::raw and temporary_buffer<char>
> seems possible to imagine.
>
> Matt

Thanks Matt. I think that's effectively what I did with 
https://github.com/cbodley/ceph/commit/67ae06e488c50a6102c66d34b942ff207493ffd6. 
It allocates a buffer::raw subclass that stores temporary_buffer<char> 
as a member. The raw allocation here is the same one we'd see with any 
other buffer type, so I was mistaken in thinking of this as extra 
overhead. The allocation in the other direction is effectively seastar's 
version of this same abstraction. So I think these conversions are fine 
as they are.

I do still think there are wins in generalizing denc to avoid these 
conversions, but we'll have to explore whether it's worth the extra 
complexity.

Re: seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

On Tue, Jan 16, 2018 at 4:48 AM, Casey Bodley <cbodley@redhat.com> wrote:
>
> On 01/15/2018 02:30 PM, Matt Benjamin wrote:
>>
>> It's the buffer::raw class that abstracts foreign buffer
>> representations now, is it now? A mechanism to allow seastar to
>> single-allocate a composite buffer::raw and temporary_buffer<char>
>> seems possible to imagine.
>>
>> Matt
>
>
> Thanks Matt. I think that's effectively what I did with
> https://github.com/cbodley/ceph/commit/67ae06e488c50a6102c66d34b942ff207493ffd6.
> It allocates a buffer::raw subclass that stores temporary_buffer<char> as a
> member. The raw allocation here is the same one we'd see with any other
> buffer type, so I was mistaken in thinking of this as extra overhead. The
> allocation in the other direction is effectively seastar's version of this
> same abstraction. So I think these conversions are fine as they are.
>
> I do still think there are wins in generalizing denc to avoid these
> conversions, but we'll have to explore whether it's worth the extra
> complexity.

i see. is https://github.com/tchaikov/ceph/tree/wip-seastar-msg
something more close to what you are thinking about?

-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[Casey Bodley]

On 01/16/2018 06:26 AM, kefu chai wrote:
> On Tue, Jan 16, 2018 at 4:48 AM, Casey Bodley <cbodley@redhat.com> wrote:
>> On 01/15/2018 02:30 PM, Matt Benjamin wrote:
>>> It's the buffer::raw class that abstracts foreign buffer
>>> representations now, is it now? A mechanism to allow seastar to
>>> single-allocate a composite buffer::raw and temporary_buffer<char>
>>> seems possible to imagine.
>>>
>>> Matt
>>
>> Thanks Matt. I think that's effectively what I did with
>> https://github.com/cbodley/ceph/commit/67ae06e488c50a6102c66d34b942ff207493ffd6.
>> It allocates a buffer::raw subclass that stores temporary_buffer<char> as a
>> member. The raw allocation here is the same one we'd see with any other
>> buffer type, so I was mistaken in thinking of this as extra overhead. The
>> allocation in the other direction is effectively seastar's version of this
>> same abstraction. So I think these conversions are fine as they are.
>>
>> I do still think there are wins in generalizing denc to avoid these
>> conversions, but we'll have to explore whether it's worth the extra
>> complexity.
> i see. is https://github.com/tchaikov/ceph/tree/wip-seastar-msg
> something more close to what you are thinking about?
>

Dear Kefu, you are a wizard. That looks like a great start!

I'm guessing that segmented buffers will be the hard part - we might 
look to the boost::asio buffer concepts for some ideas there?

they have const and non-const sequences:
http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/ConstBufferSequence.html
http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/MutableBufferSequence.html

and a dynamic/resizable sequence:
http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/DynamicBuffer.htm

Boost has lots of built-in adapters for array/vector/string types:
http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/buffer.html

If we could find a way to use those concepts directly, we'd get those 
built-in types for free, and it should be easy to adapt other types like 
those from seastar.

DynamicBuffer::prepare() looks like a natural way to do the append part 
on encode, but it returns a MutableBufferSequence so is not guaranteed 
to be contiguous. Can you remind me where we really depend on contiguous 
buffers? Is it just that some types set need_contiguous=false because we 
haven't yet implemented logic for segmented buffers, or are there cases 
where implementing that logic is actually intractable?

Thanks,
Casey

Re: seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

On Wed, Jan 17, 2018 at 6:15 AM, Casey Bodley <cbodley@redhat.com> wrote:
>
> On 01/16/2018 06:26 AM, kefu chai wrote:
>>
>> On Tue, Jan 16, 2018 at 4:48 AM, Casey Bodley <cbodley@redhat.com> wrote:
>>>
>>> On 01/15/2018 02:30 PM, Matt Benjamin wrote:
>>>>
>>>> It's the buffer::raw class that abstracts foreign buffer
>>>> representations now, is it now? A mechanism to allow seastar to
>>>> single-allocate a composite buffer::raw and temporary_buffer<char>
>>>> seems possible to imagine.
>>>>
>>>> Matt
>>>
>>>
>>> Thanks Matt. I think that's effectively what I did with
>>>
>>> https://github.com/cbodley/ceph/commit/67ae06e488c50a6102c66d34b942ff207493ffd6.
>>> It allocates a buffer::raw subclass that stores temporary_buffer<char> as
>>> a
>>> member. The raw allocation here is the same one we'd see with any other
>>> buffer type, so I was mistaken in thinking of this as extra overhead. The
>>> allocation in the other direction is effectively seastar's version of
>>> this
>>> same abstraction. So I think these conversions are fine as they are.
>>>
>>> I do still think there are wins in generalizing denc to avoid these
>>> conversions, but we'll have to explore whether it's worth the extra
>>> complexity.
>>
>> i see. is https://github.com/tchaikov/ceph/tree/wip-seastar-msg
>> something more close to what you are thinking about?
>>
>
> Dear Kefu, you are a wizard. That looks like a great start!
>
> I'm guessing that segmented buffers will be the hard part - we might look to
> the boost::asio buffer concepts for some ideas there?
>
> they have const and non-const sequences:
> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/ConstBufferSequence.html
> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/MutableBufferSequence.html
>
> and a dynamic/resizable sequence:
> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/DynamicBuffer.htm
>
> Boost has lots of built-in adapters for array/vector/string types:
> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/buffer.html
>
> If we could find a way to use those concepts directly, we'd get those
> built-in types for free, and it should be easy to adapt other types like
> those from seastar.

 i think it could be feasible to come up with some generic adapter to
use these concepts, but we might need to focus on some typical use
cases. for example, to encode and write a MOSDOpReply:

assuming that we want to encode it into multiple temporary buffers and
send them using a seastar::packet. the MOSDOpReply struct has a field
of vector<OSDOp>, and OSDOp in turn has "bufferlist outdata;" in it.
for a read op, apparently, we want to avoid memcpy. if the underlying
read(store) returns a bufferlist, the encoder should be able to
shallow copy the bufferlist and encapsulate it in a fragment with a
customized deleter or just a temporary_buffer. if we want to avoid the
conversion from buffer::list to seastar::packet, probably we should
generalize the appender so it

- is able to manage the life cycle of the buffers in bufferlist being
encoded. i.e. should flush current temporary_buffer, and translate all
buffer::ptr to seastar::temporary_buffer, then append them to the
output seastar::packet
- just writes to the local temporary_buffer directly when encoding
other kinds of objects.

so, i don't think we can have a very generic appender if we want to be
efficient. but we can define one which works for certain output type.
in this example, it's seastar::packet.

>
> DynamicBuffer::prepare() looks like a natural way to do the append part on
> encode, but it returns a MutableBufferSequence so is not guaranteed to be
> contiguous. Can you remind me where we really depend on contiguous buffers?

IIRC, we don't really depend on contiguous buffers. but we do expect
that the buffer we are manipulating is continuous, so we can have the
opportunity to optimize the dencoder:

- encoding: it's a noticeable overhead to check the underlying buffer
boundary when writing to a buffer. that's why we introduced the
appender to facilitate the encoding. see [1]
- decoding: for the similar reason, it's more performant to just read
a contiguous buffer than reading from a segmented one. in other words,
if we can ensure a the bufferlist being decoded has only a single raw
buffer, why bother checking the boundary?

if we want to encode object into the mutable buffers returned
DynamicBuffer::prepare(), we need to check x.size() when writing to
x.data() and move on to the next one if current buffer chunk is too
small.

--
[1] https://www.spinics.net/lists/ceph-devel/msg32608.html


> Is it just that some types set need_contiguous=false because we haven't yet
> implemented logic for segmented buffers, or are there cases where
> implementing that logic is actually intractable?

"need_contiguous" trait was added to the denc for the case where the
buffer being decoded are large objects and they could be segmented.
these objects are read from bluestore. but i think we would expand the
case to the large message if we rebuild/flatten the segmented buffer,
the overhead of reallocating the buffer and memcpy calls offsets the
performance improvement we achieve by avoiding the bound check and
simpler logic. before that, we always do the rebuild before decoding a
buffer::list::iterator.

>
> Thanks,
> Casey



-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[Matt Benjamin]

On Wed, Jan 17, 2018 at 7:53 AM, kefu chai <tchaikov@gmail.com> wrote:
>
>  i think it could be feasible to come up with some generic adapter to
> use these concepts, but we might need to focus on some typical use
> cases. for example, to encode and write a MOSDOpReply:
>
> assuming that we want to encode it into multiple temporary buffers and
> send them using a seastar::packet. the MOSDOpReply struct has a field
> of vector<OSDOp>, and OSDOp in turn has "bufferlist outdata;" in it.
> for a read op, apparently, we want to avoid memcpy. if the underlying
> read(store) returns a bufferlist, the encoder should be able to
> shallow copy the bufferlist and encapsulate it in a fragment with a
> customized deleter or just a temporary_buffer. if we want to avoid the
> conversion from buffer::list to seastar::packet, probably we should
> generalize the appender so it

Yes, we'd like to avoid such copies.

>
> - is able to manage the life cycle of the buffers in bufferlist being
> encoded. i.e. should flush current temporary_buffer, and translate all
> buffer::ptr to seastar::temporary_buffer, then append them to the
> output seastar::packet
> - just writes to the local temporary_buffer directly when encoding
> other kinds of objects.
>
> so, i don't think we can have a very generic appender if we want to be
> efficient. but we can define one which works for certain output type.
> in this example, it's seastar::packet.

That sounds right to me.

>
>>
>> DynamicBuffer::prepare() looks like a natural way to do the append part on
>> encode, but it returns a MutableBufferSequence so is not guaranteed to be
>> contiguous. Can you remind me where we really depend on contiguous buffers?
>
> IIRC, we don't really depend on contiguous buffers. but we do expect
> that the buffer we are manipulating is continuous, so we can have the
> opportunity to optimize the dencoder:
>
> - encoding: it's a noticeable overhead to check the underlying buffer
> boundary when writing to a buffer. that's why we introduced the
> appender to facilitate the encoding. see [1]

This is kind of surprising.

> - decoding: for the similar reason, it's more performant to just read
> a contiguous buffer than reading from a segmented one. in other words,
> if we can ensure a the bufferlist being decoded has only a single raw
> buffer, why bother checking the boundary?

Is that likely?  If the decoding case is from seastar, isn't the
incoming data likely to be in the form of several mbufs?

>
> if we want to encode object into the mutable buffers returned
> DynamicBuffer::prepare(), we need to check x.size() when writing to
> x.data() and move on to the next one if current buffer chunk is too
> small.
>
> --
> [1] https://www.spinics.net/lists/ceph-devel/msg32608.html
>
>
>> Is it just that some types set need_contiguous=false because we haven't yet
>> implemented logic for segmented buffers, or are there cases where
>> implementing that logic is actually intractable?
>
> "need_contiguous" trait was added to the denc for the case where the
> buffer being decoded are large objects and they could be segmented.
> these objects are read from bluestore. but i think we would expand the
> case to the large message if we rebuild/flatten the segmented buffer,
> the overhead of reallocating the buffer and memcpy calls offsets the
> performance improvement we achieve by avoiding the bound check and
> simpler logic. before that, we always do the rebuild before decoding a
> buffer::list::iterator.

That's what I would have expected.

>
>>
>> Thanks,
>> Casey
>
>
>
> --
> Regards
> Kefu Chai



-- 

Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-821-5101
fax.  734-769-8938
cel.  734-216-5309

Re: seastar temporary_buffer/packet and ceph::buffer

[kefu chai]

On Wed, Jan 17, 2018 at 9:13 PM, Matt Benjamin <mbenjami@redhat.com> wrote:
> On Wed, Jan 17, 2018 at 7:53 AM, kefu chai <tchaikov@gmail.com> wrote:
>>
>>  i think it could be feasible to come up with some generic adapter to
>> use these concepts, but we might need to focus on some typical use
>> cases. for example, to encode and write a MOSDOpReply:
>>
>> assuming that we want to encode it into multiple temporary buffers and
>> send them using a seastar::packet. the MOSDOpReply struct has a field
>> of vector<OSDOp>, and OSDOp in turn has "bufferlist outdata;" in it.
>> for a read op, apparently, we want to avoid memcpy. if the underlying
>> read(store) returns a bufferlist, the encoder should be able to
>> shallow copy the bufferlist and encapsulate it in a fragment with a
>> customized deleter or just a temporary_buffer. if we want to avoid the
>> conversion from buffer::list to seastar::packet, probably we should
>> generalize the appender so it
>
> Yes, we'd like to avoid such copies.
>
>>
>> - is able to manage the life cycle of the buffers in bufferlist being
>> encoded. i.e. should flush current temporary_buffer, and translate all
>> buffer::ptr to seastar::temporary_buffer, then append them to the
>> output seastar::packet
>> - just writes to the local temporary_buffer directly when encoding
>> other kinds of objects.
>>
>> so, i don't think we can have a very generic appender if we want to be
>> efficient. but we can define one which works for certain output type.
>> in this example, it's seastar::packet.
>
> That sounds right to me.
>
>>
>>>
>>> DynamicBuffer::prepare() looks like a natural way to do the append part on
>>> encode, but it returns a MutableBufferSequence so is not guaranteed to be
>>> contiguous. Can you remind me where we really depend on contiguous buffers?
>>
>> IIRC, we don't really depend on contiguous buffers. but we do expect
>> that the buffer we are manipulating is continuous, so we can have the
>> opportunity to optimize the dencoder:
>>
>> - encoding: it's a noticeable overhead to check the underlying buffer
>> boundary when writing to a buffer. that's why we introduced the
>> appender to facilitate the encoding. see [1]
>
> This is kind of surprising.

forgot to mention, preallocated buffer also avoid the possible
overhead to allocate multiple new buffers  when encoding progresses.

>
>> - decoding: for the similar reason, it's more performant to just read
>> a contiguous buffer than reading from a segmented one. in other words,
>> if we can ensure a the bufferlist being decoded has only a single raw
>> buffer, why bother checking the boundary?
>
> Is that likely?  If the decoding case is from seastar, isn't the
> incoming data likely to be in the form of several mbufs?
>
>>
>> if we want to encode object into the mutable buffers returned
>> DynamicBuffer::prepare(), we need to check x.size() when writing to
>> x.data() and move on to the next one if current buffer chunk is too
>> small.
>>
>> --
>> [1] https://www.spinics.net/lists/ceph-devel/msg32608.html
>>
>>
>>> Is it just that some types set need_contiguous=false because we haven't yet
>>> implemented logic for segmented buffers, or are there cases where
>>> implementing that logic is actually intractable?
>>
>> "need_contiguous" trait was added to the denc for the case where the
>> buffer being decoded are large objects and they could be segmented.
>> these objects are read from bluestore. but i think we would expand the
>> case to the large message if we rebuild/flatten the segmented buffer,
>> the overhead of reallocating the buffer and memcpy calls offsets the
>> performance improvement we achieve by avoiding the bound check and
>> simpler logic. before that, we always do the rebuild before decoding a
>> buffer::list::iterator.
>
> That's what I would have expected.
>
>>
>>>
>>> Thanks,
>>> Casey
>>
>>
>>
>> --
>> Regards
>> Kefu Chai
>
>
>
> --
>
> Matt Benjamin
> Red Hat, Inc.
> 315 West Huron Street, Suite 140A
> Ann Arbor, Michigan 48103
>
> http://www.redhat.com/en/technologies/storage
>
> tel.  734-821-5101
> fax.  734-769-8938
> cel.  734-216-5309



-- 
Regards
Kefu Chai

Re: seastar temporary_buffer/packet and ceph::buffer

[Casey Bodley]

On 01/17/2018 07:53 AM, kefu chai wrote:
> On Wed, Jan 17, 2018 at 6:15 AM, Casey Bodley <cbodley@redhat.com> wrote:
>> On 01/16/2018 06:26 AM, kefu chai wrote:
>>> On Tue, Jan 16, 2018 at 4:48 AM, Casey Bodley <cbodley@redhat.com> wrote:
>>>> On 01/15/2018 02:30 PM, Matt Benjamin wrote:
>>>>> It's the buffer::raw class that abstracts foreign buffer
>>>>> representations now, is it now? A mechanism to allow seastar to
>>>>> single-allocate a composite buffer::raw and temporary_buffer<char>
>>>>> seems possible to imagine.
>>>>>
>>>>> Matt
>>>>
>>>> Thanks Matt. I think that's effectively what I did with
>>>>
>>>> https://github.com/cbodley/ceph/commit/67ae06e488c50a6102c66d34b942ff207493ffd6.
>>>> It allocates a buffer::raw subclass that stores temporary_buffer<char> as
>>>> a
>>>> member. The raw allocation here is the same one we'd see with any other
>>>> buffer type, so I was mistaken in thinking of this as extra overhead. The
>>>> allocation in the other direction is effectively seastar's version of
>>>> this
>>>> same abstraction. So I think these conversions are fine as they are.
>>>>
>>>> I do still think there are wins in generalizing denc to avoid these
>>>> conversions, but we'll have to explore whether it's worth the extra
>>>> complexity.
>>> i see. is https://github.com/tchaikov/ceph/tree/wip-seastar-msg
>>> something more close to what you are thinking about?
>>>
>> Dear Kefu, you are a wizard. That looks like a great start!
>>
>> I'm guessing that segmented buffers will be the hard part - we might look to
>> the boost::asio buffer concepts for some ideas there?
>>
>> they have const and non-const sequences:
>> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/ConstBufferSequence.html
>> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/MutableBufferSequence.html
>>
>> and a dynamic/resizable sequence:
>> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/DynamicBuffer.htm
>>
>> Boost has lots of built-in adapters for array/vector/string types:
>> http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/buffer.html
>>
>> If we could find a way to use those concepts directly, we'd get those
>> built-in types for free, and it should be easy to adapt other types like
>> those from seastar.
>   i think it could be feasible to come up with some generic adapter to
> use these concepts, but we might need to focus on some typical use
> cases. for example, to encode and write a MOSDOpReply:
>
> assuming that we want to encode it into multiple temporary buffers and
> send them using a seastar::packet. the MOSDOpReply struct has a field
> of vector<OSDOp>, and OSDOp in turn has "bufferlist outdata;" in it.
> for a read op, apparently, we want to avoid memcpy. if the underlying
> read(store) returns a bufferlist, the encoder should be able to
> shallow copy the bufferlist and encapsulate it in a fragment with a
> customized deleter or just a temporary_buffer. if we want to avoid the
> conversion from buffer::list to seastar::packet, probably we should
> generalize the appender so it
>
> - is able to manage the life cycle of the buffers in bufferlist being
> encoded. i.e. should flush current temporary_buffer, and translate all
> buffer::ptr to seastar::temporary_buffer, then append them to the
> output seastar::packet
> - just writes to the local temporary_buffer directly when encoding
> other kinds of objects.
>
> so, i don't think we can have a very generic appender if we want to be
> efficient. but we can define one which works for certain output type.
> in this example, it's seastar::packet.

That example helps a lot. The asio buffer concepts aren't a good fit 
here because they don't capture our zero-copy requirements or any of the 
ownership/sharing stuff that goes along with that.

I really like how your approach only pays for the 
bufferptr->temporary_buffer conversions where we actually need that 
sharing, and not in the common encode case.

>> DynamicBuffer::prepare() looks like a natural way to do the append part on
>> encode, but it returns a MutableBufferSequence so is not guaranteed to be
>> contiguous. Can you remind me where we really depend on contiguous buffers?
> IIRC, we don't really depend on contiguous buffers. but we do expect
> that the buffer we are manipulating is continuous, so we can have the
> opportunity to optimize the dencoder:
>
> - encoding: it's a noticeable overhead to check the underlying buffer
> boundary when writing to a buffer. that's why we introduced the
> appender to facilitate the encoding. see [1]
> - decoding: for the similar reason, it's more performant to just read
> a contiguous buffer than reading from a segmented one. in other words,
> if we can ensure a the bufferlist being decoded has only a single raw
> buffer, why bother checking the boundary?
>
> if we want to encode object into the mutable buffers returned
> DynamicBuffer::prepare(), we need to check x.size() when writing to
> x.data() and move on to the next one if current buffer chunk is too
> small.
>
> --
> [1] https://www.spinics.net/lists/ceph-devel/msg32608.html

Okay, thanks Kefu. I had managed to forget most of that.

>
>> Is it just that some types set need_contiguous=false because we haven't yet
>> implemented logic for segmented buffers, or are there cases where
>> implementing that logic is actually intractable?
> "need_contiguous" trait was added to the denc for the case where the
> buffer being decoded are large objects and they could be segmented.
> these objects are read from bluestore. but i think we would expand the
> case to the large message if we rebuild/flatten the segmented buffer,
> the overhead of reallocating the buffer and memcpy calls offsets the
> performance improvement we achieve by avoiding the bound check and
> simpler logic. before that, we always do the rebuild before decoding a
> buffer::list::iterator.
>
>> Thanks,
>> Casey
>
>