libnop icon indicating copy to clipboard operation
libnop copied to clipboard

Feature request: Support for zero-copy views like std::span

Open heiner opened this issue 4 years ago • 2 comments

I have a situation where at deserialization time I'd like a view-only object like std::span (which I would give to another object's constructor where the data will be copied out). I'd like to implement my own nop::Encoding<std::span<uint8_t>> specialization for that. Unfortunately, it cannot be done on that level of abstractions since I would need to access, say, buffer_[index_] of BufferReader (https://github.com/google/libnop/blob/master/include/nop/utility/buffer_reader.h#L80).

I can instead read the prefix_byte and length at the place where I would ideally just call deserializer.Read(&myspan), get the offset via deserializer.reader().capacity() - deserializer.reader().remaining(), call deserializer.reader().Skip(length_bytes) and use the offset to index into my buffer. However, it would be nicer to encapsulate this logic.

One option would be to add a Status<void*> Ptr() { return buffer_[index_]; } method to BufferReader, and perhaps similar methods returning a failure status for other readers.

heiner avatar Jul 21 '20 13:07 heiner

I would be open to adding some form of support for view-like objects to the Reader interface. Let me consider the consequences of various options.

Hopefully you don't mind maintaining your own specialization for std::span until libnop officially adds support for C++17 standard library types?

eieio avatar Jul 23 '20 01:07 eieio

Thanks for the quick response!

And also thanks for your library, we (Facebook AI Research) are planning to use it in a number of projects.

I would be open to adding some form of support for view-like objects to the Reader interface. Let me consider the consequences of various options.

Awesome, looking forward to it!

Hopefully you don't mind maintaining your own specialization for std::span until libnop officially adds support for C++17 standard library types?

Yes definitely. I'm not actually using std::span but instead a similar view-only array that ships with PyTorch (at::ArrayRef<T>). I'm happy to write my own Encoding<at::ArrayRef<T>> for those (the documentation doesn't seem to mention this, but adding support for more types is easy enough that way).

heiner avatar Jul 23 '20 11:07 heiner