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

Send frame diffs over instead of full frames #167

Merged
merged 3 commits into from
Jun 15, 2017

Conversation

ebetica
Copy link
Contributor

@ebetica ebetica commented Jun 13, 2017

Now the connection also uses frame diffs instead of the full frames.

I ran clang-format on every file this change touched, so a lot of this is automatic, sorry guys :( I'll split it into 2 commits next time. module.cc wasn't really changed much except for the destructor.

In frame.h I removed all of the diff functions involving references, so we don't get an explosion of signatures that take either pointers or references,

Copy link
Member

@jgehring jgehring left a comment

Choose a reason for hiding this comment

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

Looks good! It's a bit hard to review the exact changes though :)

client/state.cpp Outdated
@@ -206,13 +206,28 @@ std::vector<std::string> State::update(
return upd;
}

void update_frame(State* state, const torchcraft::fbs::FrameData* fd) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should be a member function?

include/frame.h Outdated
Frame* frame_undiff(FrameDiff*, Frame*);
Frame* frame_undiff(Frame*, FrameDiff*);
void frame_undiff(Frame*, FrameDiff*, Frame*);
void frame_undiff(Frame*, Frame*, FrameDiff*);
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to provide from and to argument names here to emphasize the direction of the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undiffs (addition) is commutative :)

@ebetica
Copy link
Contributor Author

ebetica commented Jun 14, 2017

Timing until this line on bwrep_gwhxe.rep for the first 5000 frames:

With diffs:
th dump_replay.lua -t localhost 11.05s user 0.58s system 44% cpu 26.142 total

With diffs but without reserializing the frame string:
th dump_replay.lua -t localhost 3.47s user 0.54s system 21% cpu 18.475 total

Without diffs:
th dump_replay.lua -t localhost 10.80s user 0.63s system 35% cpu 32.410 total

What do we need the frame string for and can I remove it @syhw @jgehring

Copy link
Member

@syhw syhw left a comment

Choose a reason for hiding this comment

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

Looks good. You can probably remove framestring, we are not using it anymore AFAIR.

Copy link
Member

@syhw syhw left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@jgehring jgehring left a comment

Choose a reason for hiding this comment

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

Bon! Please rebase before closing, though.

@ebetica ebetica merged commit 7ec00ad into TorchCraft:develop Jun 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants