Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality so Starcoder can send back arbitrary PMTs, not just blobs #78

Merged
merged 31 commits into from
Sep 11, 2018

Conversation

reiinakano
Copy link
Contributor

@reiinakano reiinakano commented Sep 10, 2018

Adds functionality so Starcoder can send back arbitrary PMTs, not just data bytes. Instead of sending a serialized PMT, we convert it to the appropriate serialized Protobuf format and send it up the string queue.

We also introduce the Blob PMT. On the GNURadio side, this is equivalent to a uint8_vector, but since our protobuf maps the uint8_vector to a repeated uint32 field, it introduces a lot of overhead and necessitates weird conversion code when working with it from the gRPC side (e.g. converting []uint32 to []byte in Go).

@reiinakano reiinakano changed the title Convert PMT to Protocol buffer Add functionality so Starcoder can send back arbitrary PMTs, not just blobs Sep 10, 2018
BlockId: blockName,
Payload: bytes,
}); err != nil {
Pmt: message,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this code is duplicated above. Could this be made more DRY?

Copy link
Contributor

@vaelen vaelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone who knows C++ better than I do should also review this.

cqueue/string_queue.h Show resolved Hide resolved

void convert_pmt_to_proto(const pmt::pmt_t &pmt_msg,
starcoder::BlockMessage *proto_msg);
void convert_proto_uniform_vector(const pmt::pmt_t &pmt_msg,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this function is only used by convert_pmt_to_proto and does not need to be exposes in a header file.

#include <pmt/pmt.h>
#include "starcoder.pb.h"

void convert_pmt_to_proto(const pmt::pmt_t &pmt_msg,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to pass BlockMessage back as a return value. I believe the compiler is smart enough to avoid copying an object when returning it (and actually this behavior is required by the language specification.)

Copy link
Contributor Author

@reiinakano reiinakano Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I did first. The problem is the C++ generated protobuf has no way to set embedded message fields other than through getting a pointer to the embedded message field with the mutable_xxx() method and directly modifying that pointer (there is no set_xxx() method). See https://stackoverflow.com/questions/43268845/do-i-need-to-delete-objects-passed-to-google-protocol-buffer-protobuf. For the recursion to work, not sure if there's another way than having the function take a pointer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you can call Swap() on a mutable message. I haven't verified this API on my side and leave the decision up to you.
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#message

AMessageType parentMessage;

...
AMessageType submessage = convert_pmt_to_proto(...);
parentMessage.get_mutable_field()->Swap(&submessage);

....

return parentMessage;

Copy link
Contributor Author

@reiinakano reiinakano Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this would work. Is there a specific reason to prefer returning the BlockMessage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more or less a matter of personal preference so I left it up to you. But do you think it is more straightforward to return a value via a pointer argument than to use a return value? You wouldn't do it if you are programming in another language.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for reference, found these about the protobuf move semantics. Not sure if copy when reallocating affects us

protocolbuffers/protobuf#2791
protocolbuffers/protobuf#3630

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI. No explicit move is needed in this case. Its optimization for returning a local variable from a function.
https://gist.github.com/kenichi-fukushima/197238e0cd23b82666a27abfa5fbd540

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenichi-fukushima Converted this to your suggested function signature. It does seem more intuitive to use.

const std::vector<std::complex<float>> vector_elements =
pmt::c32vector_elements(pmt_msg);
std::transform(vector_elements.begin(), vector_elements.end(),
c32_vector->mutable_value()->begin(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to use a back inserter? Is it guaranteed that c32_vector has enough buffer to store all transformed elements?
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.repeated_field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I modified it. Could you check if I did it correctly? Sorry, I don't fully understand STL containers yet...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STL algorithms write to output iterators. And a vector's iterator is just the address of underlying memory buffer. So outputting to an iterator is no more than writing to a memory buffer. It won't change the container size, it won't reallocate the memory buffer if there is enough room.

const std::vector<std::complex<double>> vector_elements =
pmt::c64vector_elements(pmt_msg);
std::transform(vector_elements.begin(), vector_elements.end(),
c64_vector->mutable_value()->begin(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

u_vector->set_size(starcoder::IntSize::Size8);
const std::vector<uint8_t> vector_elements =
pmt::u8vector_elements(pmt_msg);
*u_vector->mutable_value() = { vector_elements.begin(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My C++ knowledge hasn't been update for C++11 and I really don't know how initializer expressions work... Can you explain how this assignment works? My reasoning is the right hand side calls RepeatedPtrField::RepeatedPtrField(Iter begin, const Iter & end) and then the whole content is copied for the another time, into the left hand side, which is redundant. You used transform and an inserter below. Why this piece of code follows a different pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to use std::copy. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

server/starcoder.go Outdated Show resolved Hide resolved
@reiinakano reiinakano merged commit 7c119d8 into master Sep 11, 2018
@reiinakano reiinakano deleted the convert-pmt-to-proto branch September 11, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants