cleaner serialization code #4737

kdomanski commented at 4:39 PM on August 20, 2014: contributor

The implementation of each class' serialization/deserialization is no longer passed within a macro. The implementation now lies within a template of form:

template <typename Stream, typename Operation>
inline size_t SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
    /* CODE */
}

In cases when codepath should depend on whether or not we are just deserializing (old fGetSize, fWrite, fRead flags) an additional clause can be used:

bool fRead = ser_action.ForRead();

The IMPLEMENT_SERIALIZE macro will now be a freestanding clause added within class' body (similiar to Qt's Q_OBJECT) to implement GetSerializeSize, Serialize and Unserialize. These are now wrappers around the SerializationOp template.

laanwj added the label Improvement on Aug 20, 2014

in src/serialize.h:None in e979b9fc8d outdated

  21 | @@ -22,6 +22,8 @@
  22 |  
  23 |  #include <boost/tuple/tuple.hpp>
  24 |  #include <boost/type_traits/is_fundamental.hpp>
  25 | +#include <boost/typeof/typeof.hpp>

sipa commented at 11:01 PM on August 20, 2014:

Do you use typeof anywhere?

kdomanski commented at 11:06 PM on August 20, 2014:

bool fRead = boost::is_same<Operation, CSerActionUnserialize>();

sipa commented at 11:08 PM on August 20, 2014:

Oh, right; see my next commit to get rid of that :)

in src/protocol.h:None in ba4d03ab24 outdated

 103 | +
 104 | +        template <typename Stream, typename Operation>
 105 | +        inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
 106 | +            bool fRead = ser_action.ForRead();
 107 | +
 108 | +            CAddress* pthis = const_cast<CAddress*>(this);

sipa commented at 10:18 AM on August 23, 2014:

pthis shouldn't be needed anymore.

in src/qt/recentrequeststablemodel.h:None in ba4d03ab24 outdated

  30 | +
  31 | +    template <typename Stream, typename Operation>
  32 | +    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
  33 | +        bool fRead = ser_action.ForRead();
  34 | +
  35 |          RecentRequestEntry* pthis = const_cast<RecentRequestEntry*>(this);

sipa commented at 10:19 AM on August 23, 2014:

pthis shouldn't be needed anymore.

in src/serialize.h:None in ba4d03ab24 outdated

  22 | @@ -23,6 +23,7 @@
  23 |  #include <boost/tuple/tuple.hpp>
  24 |  #include <boost/type_traits/is_fundamental.hpp>
  25 |  
  26 | +

sipa commented at 10:19 AM on August 23, 2014:

Not needed.

sipa commented at 10:21 AM on August 23, 2014: member

Untested ACK.

There are a few unnecessary constcasted pthis pointers left (more than the ones I commented on), but perhaps let's fix that afterwards, to minimize the diff here.

kdomanski commented at 10:25 AM on August 23, 2014: contributor

The pointers I added were called thisPtr. Others were here before. I'd rather not mess with them in this particular PR.

sipa commented at 10:28 AM on August 23, 2014: member

Fair enough. I was indeed referring to clutter code that was already there, that is made obsolete by the new serialization structure here. Let's not touch those for now.

sipa commented at 10:31 AM on August 23, 2014: member

Normal network sync seems to work fine.

kdomanski commented at 10:47 AM on August 23, 2014: contributor

I killed that newline.

in src/serialize.h:None in a1323f37aa outdated

 132 |  
 133 | +/* Implement three methods for serializable objects. These are actually wrappers over
 134 | + * "SerializationOp" template, which implements the body of each class' serialization
 135 | + * code. Adding "IMPLEMENT_SERIALIZE" in the body of the class causes these wrappers to be
 136 | + * added as members. */
 137 | +#define IMPLEMENT_SERIALIZE                                                                         \

TheBlueMatt commented at 10:51 PM on August 23, 2014:

Any chance you can move the \s left? They were off my screen on github and they're past 80 chars when they dont seem to need to be.

TheBlueMatt commented at 10:57 PM on August 23, 2014: member

Is it just me or are all the const_casts in serialize methods unnecessary now?

TheBlueMatt commented at 10:58 PM on August 23, 2014: member

Would it be more readable to change the name of IMPLEMENT_SERIALIZE now that its not implementing it, but just wrapping the implementation usually immediately below? Something like ADD_SERIALIZE_METHODS or so.

sipa commented at 11:15 PM on August 23, 2014: member

@TheBlueMatt Indeed, all those const_cast's are unnecessary now. To keep the patch small it doesn't hurt to leave those as-in for now (though I wouldn't object to getting rid of them either).

sipa commented at 11:19 PM on August 23, 2014: member

SerializationOp is never a const method, so it can't be called on const objects indeed. However, the added Serialize and GetSerializeSize methods added by IMPLEMENT_SERIALIZE do the un-const cast before calling SerializationOp, essentially making the assumption that GetSerializationOp will not modify the state of the object when called in serialization mode.

kdomanski commented at 9:12 AM on August 24, 2014: contributor

Moved the backslashes left.

jgarzik commented at 8:13 PM on August 27, 2014: contributor

ut ACK

laanwj referenced this in commit 333536f6db on Aug 27, 2014

sipa commented at 1:12 AM on August 30, 2014: member

Needs trivial rebase after #4778.

laanwj commented at 4:26 AM on August 30, 2014: member

Rebased commits:

laanwj@706a96b - Use CSizeComputer to avoid counting sizes in SerializationOp
laanwj@c7e2682 - rework overhauled serialization methods to non-static
laanwj@45573d5 - remove fields of ser_streamplaceholder
laanwj@e9fff52 - overhaul serialization code

jtimon commented at 10:57 AM on August 30, 2014: contributor

Idea ack

overhaul serialization code

The implementation of each class' serialization/deserialization is no longer
passed within a macro. The implementation now lies within a template of form:

template <typename T, typename Stream, typename Operation>
inline static size_t SerializationOp(T thisPtr, Stream& s, Operation ser_action, int nType, int nVersion) {
    size_t nSerSize = 0;
    /* CODE */
    return nSerSize;
}

In cases when codepath should depend on whether or not we are just deserializing
(old fGetSize, fWrite, fRead flags) an additional clause can be used:
bool fRead = boost::is_same<Operation, CSerActionUnserialize>();

The IMPLEMENT_SERIALIZE macro will now be a freestanding clause added within
class' body (similiar to Qt's Q_OBJECT) to implement GetSerializeSize,
Serialize and Unserialize. These are now wrappers around
the "SerializationOp" template.

3d796f8996

remove fields of ser_streamplaceholder

The nType and nVersion fields of stream objects are never accessed
from outside the class (or perhaps from the inside too, I haven't checked).
Thus no need to have them in a placeholder, whose only purpose is to
fill the "Stream" template parameter in serialization implementation.

5d96b4ae01

rework overhauled serialization methods to non-static

Thanks to Pieter Wuille for most of the work on this commit.
I did not fixup the overhaul commit, because a rebase conflicted
with "remove fields of ser_streamplaceholder".
I prefer not to risk making a mistake while resolving it.

84881f8c47

Use CSizeComputer to avoid counting sizes in SerializationOp 31e9a8384a

BitcoinPullTester commented at 12:34 AM on August 31, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4737_31e9a8384a77947f6777d035992f4734618ed206/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 1:00 PM on September 1, 2014: member

@laanwj Is that an ACK?

laanwj commented at 1:58 PM on September 1, 2014: member

@sipa I'm testing it on my own nodes, hence the rebases. ACK on the code changes, and haven't encountered any problems yet.

sipa merged this on Sep 1, 2014

sipa closed this on Sep 1, 2014

sipa referenced this in commit 2e731f24b5 on Sep 1, 2014

kdomanski deleted the branch on Sep 1, 2014

MarcoFalke locked this on Sep 8, 2021