Hi,

I think it might be a good idea to use Protobuf.net in the future for message serialization. It's used in nearly everything (including most of Google and Steam) and would be particularly good here for its backwards-compatibility. Rather than enforcing a specific protocol version you could gracefully handle API updates with protobuf using optional fields. Also, binary serialization is very fast and small and could help lower your bandwidth a bit.

Thoughts?

I've started a branch in my fork called protobuf. It's only going to take me around an hour and a half to convert the entire project to Protobuf. It's already a lot cleaner - we have hard types for every message, an easy to read protocol definition:

//All of the DMP message types
enum EDMPServerMessageType {
    HEARTBEAT = 0;
    HANDSHAKE_CHALLANGE = 1;
    HANDSHAKE_REPLY = 2;
    SERVER_SETTINGS = 3;
    CHAT_MESSAGE = 4;
    PLAYER_STATUS = 5;
    PLAYER_COLOR = 6;
    PLAYER_JOIN = 7;
    PLAYER_DISCONNECT = 8;
    SCENARIO_DATA = 9;
    KERBAL_REPLY = 10;
    KERBAL_COMPLETE = 11;
    VESSEL_LIST = 12;
    VESSEL_PROTO = 13;
    VESSEL_UPDATE = 14;
    VESSEL_COMPLETE = 15;
    VESSEL_REMOVE = 16;
    CRAFT_LIBRARY = 17;
    SCREENSHOT_LIBRARY = 18;
    FLAG_SYNC = 19;
    SET_SUBSPACE = 20;
    SYNC_TIME_REPLY = 21;
    PING_REPLY = 22;
    MOTD_REPLY = 23;
    WARP_CONTROL = 24;
    ADMIN_SYSTEM = 25;
    LOCK_SYSTEM = 26;
    MOD_DATA = 27;
    SPLIT_MESSAGE = 28;
    CONNECTION_END = 29;
}

//The base server message container
message DMPServerMessage {
    required EDMPServerMessageType type = 1;
    required bytes data = 2;
}

I'll do some testing later today.

The problem with doing this is that you keep useless fields inside of the message, and fields sometimes change their meaning. I'd much rather force a protocol break on people and keep the message size down - it's not too much of an ask for people to update on a break.

We had major troubles with KMP and BinarySerialiser so that's why I've stayed away from it. I'd have to look into how protobuff does things before I'd accept a PR to it.

I do admit, my messagewriter isn't perfect, but it serves its purpose :)

The Protobuf wire protocol is far far more efficient than BinarySerializer and also has better compression for less data usage.

That's actually not true that the old fields are kept. You just delete the field and don't use that integer field identifier in the future. Keeps the code pretty clean.

If the field changes its meaning you would want to move it to a new integer identifier.

With any huge breaking change you would increment the protocol number, but this allows for more minor iterations without much more than a few edge case bugs being introduced with version mismatches.

There's nothing more efficent than bit-packing the fields, which is exactly what messagewriter does :-/

You're very right. I'm just suggesting a strong protocol definition might be a smart development decision :) It's not going to impact performance at all, I assure you.

I've taken some measurements. I have a protobuf protocol definition with most everything from common.

Some of the enums:

enum EGameMode {
    SANDBOX = 0;
    SCIENCE = 1;
    CAREER = 2;
}

enum EModControlMode {
    DISABLED_MOD_CONTROL = 0;
    ENABLED_STOP_INVALID_PART_SYNC = 1;
    ENABLED_STOP_INVALID_PART_LAUNCH = 2;
}

enum EChatMessageType {
    LIST = 0;
    JOIN = 1;
    LEAVE = 2;
    CHANNEL_MESSAGE = 3;
    PRIVATE_MESSAGE = 4;
    CONSOLE_MESSAGE = 5;
}

enum ELockMessageType {
    LOCK_LIST = 0;
    LOCK_ACQUIRE = 1;
    LOCK_RELEASE = 2;
}

These are generated to c# code that works with protobuf-net.

Here's what a message looks like:

message HandshakeResponse {
    //increment this on major breaking changes to DMP
    required int32 major_version = 1;
    optional string player_name = 2;
    optional string player_public_key = 3;
    optional bytes challenge_signature = 4;
    optional string client_version = 5;
    optional string reason = 6;
}

Note that optional does not mean the client won't include it, but rather means the field could be eliminated in later versions of the protocol without throwing errors on older/newer clients. As Google says - " You should be very careful about marking fields as required. If at some point you wish to stop writing or sending a required field, it will be problematic to change the field to an optional field – old readers will consider messages without this field to be incomplete and may reject or drop them unintentionally. You should consider writing application-specific custom validation routines for your buffers instead. Some engineers at Google have come to the conclusion that using required does more harm than good; they prefer to use only optional and repeated. However, this view is not universal.".

With major breaking versions of the program we can increment major_version. With minor changes like new fields it's not necessary; the protocol will handle new fields missing gracefully. With major changes you can still reject the connection.

Finally, speed. I know the current implementation bit packs the data, which is as efficient as you can get, however, you have a tradeoff: terrible version history support, lack of a documented protocol, and lack of drop-in cross language support you have in Protobuf. You also cannot handle protocol changes gracefully, and you can run into some serious issues and complexity down the line. The serialization / deserialization in protobuf takes 100-200 nanoseconds.

Overall it's not a major change and you won't see a huge increase/decrease in speed or bandwidth, but you're introducing more development structure with the added benefit of small revisions not requiring a client update, cross language compatibility, strong typing for your messages (easier deserialization and protocol revisions), and in general a trusted protocol system used in a grand number of applications.

Well why not take your branch and the latest Dev and take some measurements. If cross version compatibility is essentially guaranteed, great. If it reduces bandwidth and/or packet reliability, why not. It all depends on how it works in a practical implementation.

If you think it's the same thing as BinaryFormatter you really do need to do some more research.

The primary advantage of something like protobuf is that you have a strongly defined protocol which avoids all of the issues with BinaryFormatter and generic bit packing. You can also easily reject invalid messages which might be a bit of a problem with reading bit packed messages.

Rejecting this - I really do not want to keep compatibility code around forever, and that's exactly what protobuff will cause if I try to remain compatible. It's not too much of an ask clients upgrade for new versions, and allowing clients to stay on older versions is almost certainly a bad thing.

And older protocol versions are just that, old. I'm not interested in the history of DMP, nor do I want to try and handle old messages gracefully :-/.

I think you completely misunderstand what protobuf does... It's not a compatibility code engine, it's just a side effect of the way it works. You can still enforce up-to-date versions just as you are now.

I suggest you research it a bit before rejecting it

@paralin: In that case, there's no reason not to use required and just bump PROTOCOL_VERSION as before. Which means your touted compatibility advantages are all lost ;).

I think I see what you're getting at - You're talking about removing messagewriter and just feeding objects directly into a serialiser. But in that case, I can't see any difference to something like BinaryFormatter - And I've been burned by that before in KMP.

EDIT: It may be helpful to visit on IRC - I wouldn't mind seeing your point of view, but it takes a fair bit for me to accept an external dependency when I already have a working solution.