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 serialization #25

Closed
wants to merge 15 commits into from
Closed

Conversation

yuvalif
Copy link

@yuvalif yuvalif commented Dec 18, 2018

Adding circular buffer serialization (including the space optimized version). This is to address issue #24.
For performance reasons the serialization of the buffer itself is done in, bulk, in binary format, and not element by element.

Copy link

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Licensing issues...

example/circular_buffer_serialization.cpp Show resolved Hide resolved
@@ -12,6 +12,7 @@ project
: requirements
<library>/boost/system//boost_system
<library>/boost/thread//boost_thread
<library>/boost//serialization
Copy link

Choose a reason for hiding this comment

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

This doesn't match the others - is it correct?

Copy link
Author

Choose a reason for hiding this comment

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

you mean the library location? i don't know why it is different, but this is the correct one for serialization

include/boost/circular_buffer/serialization.hpp Outdated Show resolved Hide resolved
@glenfe
Copy link
Member

glenfe commented Feb 25, 2019

This would make circular_buffer have a dependency on serialization which I'm not sure we want.

@jeking3
Copy link

jeking3 commented Feb 26, 2019

It would be easy enough to include macro guards to disable it entirely if someone so desired...

@yuvalif
Copy link
Author

yuvalif commented Feb 26, 2019

@glenfe @jeking3 , in multi_index it was done under: BOOST_MULTI_INDEX_DISABLE_SERIALIZATION
will follow the same pattern here

Signed-off-by: Yuval Lifshitz <yuvalif@yahoo.com>
@pdimov
Copy link
Member

pdimov commented Feb 26, 2019

The tests only test a completely full buffer consisting of one span. In general, an archive does not allow two save_binary calls on save to be loaded with one load_binary.

Using binary serialization is questionable anyway. This only works for trivially copyable types, and the code doesn't check that; and text/xml archives are supposed to be portable, and dumping binary into them defeats their purpose. This implementation is only suitable for use with the nonportable binary archive.

@yuvalif
Copy link
Author

yuvalif commented Feb 26, 2019

@pdimov

The tests only test a completely full buffer consisting of one span.

Fragmented buffer was tested in the examples, will add to the unit test.

In general, an archive does not allow two save_binary calls on save to be loaded with one load_binary.

Didn't want to use serialize,to avoid changes to the buffer. Can change to 2 separate calls of load_binary. BTW, why is there an issue with 2 calls?

In general binary serialization was used for performance reasons, but I do see the issues with it. Will modify the implementation archive the elements one by one.

There is probably a way to specialize for the case that the archive is binary anyway - will look into that.

@pdimov
Copy link
Member

pdimov commented Feb 26, 2019

BTW, why is there an issue with 2 calls?

Well, suppose that some XML archive implements save_binary as <binary>04A3719F...</binary>. The two save_binary calls will emit two such sequences. Then the load_binary call will only load the first one, and it won't have enough data inside.

Signed-off-by: Yuval Lifshitz <yuvalif@yahoo.com>
Signed-off-by: Yuval Lifshitz <yuvalif@yahoo.com>
Signed-off-by: Yuval Lifshitz <yuvalif@yahoo.com>
@yuvalif
Copy link
Author

yuvalif commented Mar 4, 2019

  • added disabling macro: BOOST_CIRCULAR_BUFFER_DISABLE_SERIALIZATION for the feature. @glenfe @jeking3 does it needs to be documented somewhere?
  • added test for non-linearized buffer (which actually found a bug...)
  • basic implementation serialize the elements one by one, added specialized implementation for the binary archive case. @pdimov please let me know if you still see issue with this optimization - if this is becoming too complex I can remove (note that there is a big difference in performance, shown in the example)
  • note that XML archive is not supported, as I don't use NVP serialization. Don't think this is mandatory in the circular buffer case. And could be added later

Signed-off-by: Yuval Lifshitz <yuvalif@yahoo.com>
@jeking3
Copy link

jeking3 commented Mar 5, 2019

Documentation is useful, so if you can add it to the docs (BOOST_CIRCULAR_BUFFER_DISABLE_SERIALIZATION), I think that's best.

@yuvalif yuvalif closed this Jul 12, 2022
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