Skip to content
This repository has been archived by the owner on Jul 26, 2022. It is now read-only.

Port TorchCraft communication frames from txt to binary #213

Closed
syhw opened this issue Oct 31, 2017 · 7 comments
Closed

Port TorchCraft communication frames from txt to binary #213

syhw opened this issue Oct 31, 2017 · 7 comments
Assignees

Comments

@syhw
Copy link
Member

syhw commented Oct 31, 2017

Run all TC tests + test with the bot.

@dgant dgant self-assigned this Oct 31, 2017
@ebetica
Copy link
Contributor

ebetica commented Oct 31, 2017

The test we usually do for anything related to the replay format is here: https://github.com/TorchCraft/TorchCraft/blob/master/examples/dump_replay.lua

@dgant
Copy link
Contributor

dgant commented Oct 31, 2017

Seconding @jgehring 's suggestion of FlatBuffers. It has the third-fastest round-trip serialization among these benchmarked libraries: https://github.com/thekvs/cpp-serializers (accepting the usual disclaimers about the utility of benchmarks).

For FlatBuffers, that'd entail:

  • Adding the FlatBuffers library
  • Adding a schema for the Replayer
  • Adding a script for compiling the schema
  • Including the schema compilation in the TC build script
  • Replacing the DSV serialization with the FlatBuffers serialization

Thumbs, @syhw @jgehring @ebetica ?

@syhw
Copy link
Member Author

syhw commented Oct 31, 2017

We already use flatbuffers for some stuff https://github.com/TorchCraft/TorchCraft/tree/master/BWEnv/fbs I think that's the point of the remark of @jgehring . My point of view: "Yay go FBs!"

@jgehring
Copy link
Member

Yup, the current wire protocol is using fbs for message serialization, so it's a natural choice. One particular advantage (in general) is its zero-overhead deserialization which makes it a great option for e.g. ML datasets. At the path that @syhw is pointing to, there's already a small script which prepares the fbs header file. We keep that one in git directly (and also include the fbs header) so that we don't introduce another build-time dependency.

The frame-related serialization methods are located in replayer/frame.cpp. We can use fbs for the replayer as well, but as a first step it might be easier to use fbs-serialized frames for client-server communication only (basically, having meaningful structs for Frame.data in messages.fbs and doing serialization in BWEnv and deserialization in TC client). As you said, we can then also port all of replayer, including frame diffs, to fbs. Note that the current struct-based setup of replayer::Frame requires copying the data from the fbs struct if we don't want to lose compatibility (which is what we should aim for at this stage).

One caveat (if you're on macOS) is that IIRC @syhw mentioned that the recent fbs version from Homebrew is somehow borked; I can't remember the details though.

@dgant
Copy link
Contributor

dgant commented Nov 1, 2017

What are we hoping to maintain compatibility with? StarData?

@ebetica
Copy link
Contributor

ebetica commented Nov 1, 2017

StarData already doesn't work on develop. I think it's fine to just replace everything, but I might be missing something obvious.

@jgehring
Copy link
Member

jgehring commented Nov 1, 2017

What are we hoping to maintain compatibility with? StarData?

I wanted to point out that changing the actual data-structures as opposed to just the serialization format is a bigger effort and affects library users, scripting frontends, etc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants