Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(13)

Issue 6357056: Generic hash function interface, with two implementations. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 9 months ago by Peter Barnes
Modified:
6 years, 3 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

This is the first of three of patches to address Bug 582, "Tags are not serialized and deserialized from Packet::Serialize and Packet::Deserialize" https://www.nsnam.org/bugzilla/show_bug.cgi?id=582 One issue is to serialize the *type* of the derived Tag, when the Packet only has a PacketTagList to work from. This serialization has to produce a consistent result on every compute node in a parallel cluster, even one with heterogeneous nodes. This can be particularly tricky if the different instances of ns3 don't create TypeId's in the same order, which results in a single type (by name) being represented by different TypeId values (integers) on different nodes. One approach, partially implemented by Jeffrey Young at Georgia Tech, is to serialize the TypeId name (a string). This seems unattractive because it takes time and space, typically much more space than the tag data itself (which is limited to 20 bytes). The alternative approach proposed here is to add a consistent 32-bit hash to the TypeId, then pass that to identify the tag type. To simplify review, I've split this into three patches: 1. Add class Hash: generic hash function interface, with two implementations (this review). 2. Add hashes to TypeId. 3. Use TypeId hash to serialize Tags. This review covers the Hash class and implementation. The class itself provides a simple API for callers to hash objects. It wraps multiple underlying hash function implementations. This patch includes the venerable FNV1a, and murmur3. It is straightforward to add new implementations. If your hash function has the right signature, it can be as simple as: Hash (Ptr<HashImplementation> (Hash32Implementation<&hashf>))

Patch Set 1 #

Total comments: 8

Patch Set 2 : Respond to comments, add incremental hashing, fix hash function ptr, additional test cases, check-s… #

Total comments: 2

Patch Set 3 : Respond to review comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+668 lines, -215 lines) Patch
M doc/manual/source/hash-functions.rst View 1 2 2 chunks +55 lines, -11 lines 0 comments Download
M src/core/model/hash.h View 1 2 12 chunks +62 lines, -27 lines 1 comment Download
M src/core/model/hash.cc View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M src/core/model/hash-fnv.h View 1 2 2 chunks +45 lines, -11 lines 0 comments Download
M src/core/model/hash-fnv.cc View 1 2 4 chunks +28 lines, -20 lines 0 comments Download
M src/core/model/hash-function.h View 1 2 4 chunks +46 lines, -24 lines 0 comments Download
M src/core/model/hash-function.cc View 1 2 1 chunk +6 lines, -6 lines 0 comments Download
M src/core/model/hash-murmur3.h View 1 2 3 chunks +41 lines, -14 lines 0 comments Download
M src/core/model/hash-murmur3.cc View 1 2 7 chunks +114 lines, -28 lines 0 comments Download
M src/core/test/hash-test-suite.cc View 1 2 3 chunks +265 lines, -68 lines 0 comments Download
M src/core/wscript View 1 2 4 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 12
Jeff Y
Peter, This looks good to me at least in terms of the coding style, and ...
8 years, 8 months ago (2012-07-23 14:19:42 UTC) #1
Peter Barnes
http://codereview.appspot.com/6357056/diff/1/doc/manual/source/hash-functions.rst File doc/manual/source/hash-functions.rst (right): http://codereview.appspot.com/6357056/diff/1/doc/manual/source/hash-functions.rst#newcode71 doc/manual/source/hash-functions.rst:71: * SMHasher: On 2012/07/23 14:19:42, Jeff Y wrote: > ...
8 years, 8 months ago (2012-07-23 18:45:29 UTC) #2
Peter Barnes
8 years, 8 months ago (2012-07-25 23:40:32 UTC) #3
Peter Barnes
On 2012/07/25 23:40:32, Peter Barnes wrote: Looks like a few ns-3-dev changes snuck in to ...
8 years, 8 months ago (2012-07-25 23:48:31 UTC) #4
Jeff Y
Patch Set 2 looks good. Thanks for going ahead and adding support and test cases ...
8 years, 8 months ago (2012-07-26 14:50:13 UTC) #5
Mathieu Lacage
one minor comment. nothing holding commit. Great code ! http://codereview.appspot.com/6357056/diff/7001/src/core/model/hash-function.h File src/core/model/hash-function.h (right): http://codereview.appspot.com/6357056/diff/7001/src/core/model/hash-function.h#newcode33 src/core/model/hash-function.h:33: ...
8 years, 8 months ago (2012-07-28 07:43:11 UTC) #6
Mathieu Lacage
ack
8 years, 8 months ago (2012-07-28 08:01:39 UTC) #7
Peter Barnes
On 2012/07/28 07:43:11, Mathieu Lacage wrote: > http://codereview.appspot.com/6357056/diff/7001/src/core/model/hash-function.h#newcode33 > src/core/model/hash-function.h:33: typedef uint32_t Hash32_t; > it ...
8 years, 7 months ago (2012-08-20 19:40:17 UTC) #8
Peter Barnes
8 years, 7 months ago (2012-08-20 23:22:10 UTC) #9
Peter Barnes
http://codereview.appspot.com/6357056/diff/7001/src/core/wscript File src/core/wscript (right): http://codereview.appspot.com/6357056/diff/7001/src/core/wscript#newcode127 src/core/wscript:127: 'model/random-variable-stream.cc', Not sure how these edits got in here; ...
8 years, 7 months ago (2012-08-20 23:32:58 UTC) #10
Peter Barnes
7 years, 8 months ago (2013-07-24 21:13:30 UTC) #11
Tom Henderson
7 years, 8 months ago (2013-07-25 04:31:46 UTC) #12
On 2013/07/24 21:13:30, Peter Barnes wrote:

Peter, can you refresh us on the status of this and the associated bug.?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b