Skip to content

serialization performance improvements #361

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,21 @@ class OMG_DDS_API cdr_stream {
*/
virtual bool finish_member(const entity_properties_t &props, member_id_set &member_ids, bool is_set = true);

/**
* @brief
* Function declaration for finishing an existing member.
*
* This function is called by next_entity for each entity which is iterated over.
* Depending on the implementation and mode header length fields may be completed.
* This function can be overridden in cdr streaming implementations.
*
* @param[in] props Properties of the member to finish.
Copy link
Contributor

Choose a reason for hiding this comment

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

So presumably the _unchecked refers to it not outputting a set of members that were completed successfully (the member_ids) in the original one. I presume that for both functions the return value is false iff some failure occurred, and that the nice bit of the non-unchecked version is that you get more detailed information on what actually happened.

Ok, that makes sense.

What I do wonder is if it would not make more sense to overload them? If the only difference is getting some additional information out, then it seems like that would more elegant and introduce less risk of head-scratching.

Would you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _unchecked versions of the finish_member and finish_struct functions do not check the member or struct for completeness (whether all members are present) when doing a streaming operation.
These functions do the streaming steps when finishing a struct or a member, like reading/inserting headers etc.
The check for completeness is not necessary, unless reading a non-final extensibility struct, since there we have no control over the set of members present.
This check uses std::set as a container of the member ids that were read, and this was a significant performance drain, and therefore was split out for those datatype+streaming operation combinations that needed it.

Overloading is indeed an option, i will look in to this, this should also simplify the code generator.
Don't know why I overlooked this when writing this, probably went into this with a little too much C code attitude.

* @param[in] is_set Whether the entity represented by prop is present, if it is an optional entity.
*
* @return Whether the operation was completed succesfully.
*/
virtual bool finish_member_unchecked(const entity_properties_t &props, bool is_set = true);

/**
* @brief
* Function declaration for retrieving the next entity to be operated on by the streamer.
Expand Down Expand Up @@ -481,7 +496,20 @@ class OMG_DDS_API cdr_stream {
*
* @return Whether the struct is complete and correct.
*/
virtual bool finish_struct(const entity_properties_t &props, const member_id_set &member_ids);
virtual bool finish_struct(const entity_properties_t &props, const member_id_set &member_ids) {return check_struct_completeness(props, member_ids);}

/**
* @brief
* Function declaration for finishing a parameter list.
*
* This function is called by the generated functions for the entity, and will trigger the necessary actions on finishing the current struct.
* I.E. finishing headers, writing length fields.
*
* @param[in] props The entity whose members might be represented by a parameter list.
*
* @return Whether the struct is complete and correct.
*/
virtual bool finish_struct_unchecked(const entity_properties_t &props) {(void)props; return true;}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar reasoning to the finish_member_unchecked applies here (perhaps there are other places as well, I won't repeat this if there are, that'd be silly).

I am cool with being wrong, mind 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, the explanation is included in the previous comment.


/**
* @brief
Expand Down Expand Up @@ -941,6 +969,39 @@ bool max_string(S& str, const T& max_sz, size_t N)
return true;
}

/**
* @brief
* Sequence of base types read function.
*
* This is called when a resize + memcpy is way less efficient than an assign,
* as this skips the default initialization of entities in the sequence.
*
* @param[in, out] str The stream being read from.
* @param[in] toread The sequence being read into.
* @param[in] N Number of base entities to read.
*
* @return Whether the operation was completed succesfully.
*/
template<typename S, template<typename,typename> class V, typename T, typename A, std::enable_if_t<std::is_base_of<cdr_stream, S>::value && std::is_arithmetic<T>::value && !std::is_same<T,bool>::value, bool> = true >
bool read(S& str, V<T,A> &toread, size_t N)
{
if (str.position() == SIZE_MAX
|| !str.align(sizeof(T), false)
|| !str.bytes_available(sizeof(T)*N))
return false;

auto ptr = reinterpret_cast<const T*>(str.get_cursor());
toread.assign(ptr, ptr+N);
str.incr_position(N*sizeof(T));

if (str.swap_endianness()) {
for (auto &e:toread)
byte_swap(&e);
}

return true;
}

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,11 @@ class OMG_DDS_API xcdr_v1_stream : public cdr_stream {
* Determines whether a header is necessary for this entity through header_necessary, and if it is, completes the previous header.
*
* @param[in] prop Properties of the member to finish.
* @param[in] member_ids Container for the member ids of members succesfully streamed at this level
* @param[in] is_set Whether the entity represented by prop is present, if it is an optional entity.
*
* @return Whether the operation was completed succesfully.
*/
bool finish_member(const entity_properties_t &prop, member_id_set &member_ids, bool is_set = true);
bool finish_member_unchecked(const entity_properties_t &prop, bool is_set = true);

/**
* @brief
Expand Down Expand Up @@ -99,11 +98,10 @@ class OMG_DDS_API xcdr_v1_stream : public cdr_stream {
* Adds the final parameter list entry if necessary when writing to the stream.
*
* @param[in] props The property tree to get the next entity from.
* @param[in] member_ids Container for the member ids of members succesfully streamed at this level
*
* @return Whether the struct is complete and correct.
*/
bool finish_struct(const entity_properties_t &props, const member_id_set &member_ids);
bool finish_struct_unchecked(const entity_properties_t &props);

private:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,11 @@ class OMG_DDS_API xcdr_v2_stream : public cdr_stream {
* Determines whether a header is necessary for this entity through em_header_necessary, and if it is, completes the previous header.
*
* @param[in] prop Properties of the member to finish.
* @param[in] member_ids Container for the member ids of members succesfully streamed at this level
* @param[in] is_set Whether the entity represented by prop is present, if it is an optional entity.
*
* @return Whether the operation was completed succesfully.
*/
bool finish_member(const entity_properties_t &prop, member_id_set &member_ids, bool is_set = true);
bool finish_member_unchecked(const entity_properties_t &prop, bool is_set = true);

/**
* @brief
Expand Down Expand Up @@ -123,6 +122,18 @@ class OMG_DDS_API xcdr_v2_stream : public cdr_stream {
*/
bool finish_struct(const entity_properties_t &props, const member_id_set &member_ids);

/**
* @brief
* Finish the current struct.
*
* This function is called by the generated streaming functions, and will finish the current parameter list, if that is relevant for it.
*
* @param[in] props The entity whose members might be represented by a parameter list.
*
* @return Whether the struct is complete and correct.
*/
bool finish_struct_unchecked(const entity_properties_t &props);

/**
* @brief
* Function declaration for starting an array or sequence of non-primitive types.
Expand Down
19 changes: 8 additions & 11 deletions src/ddscxx/src/org/eclipse/cyclonedds/core/cdr/cdr_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,6 @@ bool cdr_stream::align(size_t newalignment, bool add_zeroes)
return true;
}

bool cdr_stream::finish_struct(const entity_properties_t &props, const member_id_set &member_ids)
{
switch (m_mode) {
case stream_mode::read:
return check_struct_completeness(props, member_ids);
break;
default:
return true;
}
}

const entity_properties_t *cdr_stream::first_entity(const entity_properties_t *props)
{
const entity_properties_t *prop = props->first_member;
Expand Down Expand Up @@ -140,11 +129,19 @@ bool cdr_stream::check_struct_completeness(const entity_properties_t &props, con

bool cdr_stream::finish_member(const entity_properties_t &props, member_id_set &member_ids, bool is_set)
{
if (!finish_member_unchecked(props, is_set))
return false;

if (is_set)
member_ids.insert(props.m_id);
return true;
}

bool cdr_stream::finish_member_unchecked(const entity_properties_t &, bool)
{
return true;
}

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ bool xcdr_v1_stream::start_member(const entity_properties_t &prop, bool is_set)
return cdr_stream::start_member(prop, is_set);
}

bool xcdr_v1_stream::finish_member(const entity_properties_t &prop, member_id_set &member_ids, bool is_set)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct that finish_member is no longer needed because cdr_stream::finish_member takes care of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the functionality which was duplicated here is now absorbed into the base class's version of the function.

bool xcdr_v1_stream::finish_member_unchecked(const entity_properties_t &prop, bool is_set)
{
if (header_necessary(prop)) {
switch (m_mode) {
Expand All @@ -73,7 +73,7 @@ bool xcdr_v1_stream::finish_member(const entity_properties_t &prop, member_id_se
}

pop_member_start();
return cdr_stream::finish_member(prop, member_ids, is_set);
return cdr_stream::finish_member_unchecked(prop, is_set);
}

const entity_properties_t* xcdr_v1_stream::next_entity(const entity_properties_t *prop)
Expand Down Expand Up @@ -288,7 +288,7 @@ bool xcdr_v1_stream::finish_write_header(const entity_properties_t &props)
return true;
}

bool xcdr_v1_stream::finish_struct(const entity_properties_t &props, const member_id_set &member_ids)
bool xcdr_v1_stream::finish_struct_unchecked(const entity_properties_t &props)
{
switch (m_mode) {
case stream_mode::write:
Expand All @@ -300,11 +300,8 @@ bool xcdr_v1_stream::finish_struct(const entity_properties_t &props, const membe
if (list_necessary(props))
return move_final_list_entry();
break;
case stream_mode::read:
return check_struct_completeness(props, member_ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't yet figure out how check_struct_completeness role is now being fulfilled (also applies to xcdr_v2_stream, obviously).

break;
default:
return false;
break;
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ bool xcdr_v2_stream::start_member(const entity_properties_t &prop, bool is_set)
return cdr_stream::start_member(prop, is_set);
}

bool xcdr_v2_stream::finish_member(const entity_properties_t &prop, member_id_set &member_ids, bool is_set)
bool xcdr_v2_stream::finish_member_unchecked(const entity_properties_t &prop, bool is_set)
{
switch (m_mode) {
case stream_mode::write:
Expand All @@ -83,7 +83,7 @@ bool xcdr_v2_stream::finish_member(const entity_properties_t &prop, member_id_se

m_consecutives.pop();
pop_member_start();
return cdr_stream::finish_member(prop, member_ids, is_set);
return cdr_stream::finish_member_unchecked(prop, is_set);
}

bool xcdr_v2_stream::write_d_header()
Expand Down Expand Up @@ -313,21 +313,19 @@ bool xcdr_v2_stream::start_struct(const entity_properties_t &props)

bool xcdr_v2_stream::finish_struct(const entity_properties_t &props, const member_id_set &member_ids)
{
switch (m_mode) {
case stream_mode::write:
if (d_header_necessary(props) && !finish_d_header())
return false;
break;
case stream_mode::read:
if (!check_struct_completeness(props, member_ids))
return false;
else if (d_header_necessary(props))
m_buffer_end.pop();
break;
default:
break;
}
if (!cdr_stream::finish_struct(props, member_ids))
return false;
else if (d_header_necessary(props))
m_buffer_end.pop();

return true;
}

bool xcdr_v2_stream::finish_struct_unchecked(const entity_properties_t &props)
{
if (stream_mode::write == m_mode && d_header_necessary(props) && !finish_d_header())
return false;

return true;
}

Expand Down
Loading