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

Rajan/preamble fb #24

Merged
merged 25 commits into from
Dec 6, 2018
Merged

Rajan/preamble fb #24

merged 25 commits into from
Dec 6, 2018

Conversation

rajan-chari
Copy link
Member

  • Flatbuffer
  • Object pooling and memory management for FB
  • Preamble changes
  • Manually merged master

build-linux.sh Outdated Show resolved Hide resolved
include/data_buffer.h Show resolved Hide resolved
include/data_buffer.h Outdated Show resolved Hide resolved
include/data_buffer.h Outdated Show resolved Hide resolved
rlclientlib/ranking_event.cc Outdated Show resolved Hide resolved
rlclientlib/schema/RankingEvent.fbs Outdated Show resolved Hide resolved
rlclientlib/serialization/fb_serializer.h Outdated Show resolved Hide resolved
rlclientlib/utility/data_buffer_streambuf.h Outdated Show resolved Hide resolved
rlclientlib/utility/object_pool.h Show resolved Hide resolved
include/data_buffer.h Outdated Show resolved Hide resolved
Copy link
Contributor

@tyclintw tyclintw left a comment

Choose a reason for hiding this comment

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

:shipit:

using offset_vector_t = typename std::vector<flatbuffers::Offset<fb_event_t>>;
using batch_builder_t = RankingEventBatchBuilder;

static size_t size_estimate(const ranking_event& evt) {
Copy link
Member

@ataymano ataymano Dec 3, 2018

Choose a reason for hiding this comment

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

This size estimation is required for controlling size of queue in memory (before serialization), and there is nothing fb-specific in implementation as well. So, probably it should not be part of serializer? We have static assert that any event have to be inherited from base event class, so probably it is cleaner to add size() method to that interface? In this case we also can get rid of size argument for event_queue::push.

@rajan-chari rajan-chari merged commit df76117 into VowpalWabbit:master Dec 6, 2018
@rajan-chari rajan-chari deleted the rajan/preamble_fb branch December 6, 2018 11: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