Store commit message in linked list instead of map

Author: qdm12

lib/grandpa/commits_tracker.go

@@ -14,26 +14,19 @@ import (
 // its maximum capacity is reached.
 // It is NOT THREAD SAFE to use.
 type commitsTracker struct {
-	// map of commit block hash to data
-	// data = message + tracking linked list element pointer
-	mapping map[common.Hash]commitMessageMapData
-	// double linked list of block hash
+	// map of commit block hash to linked list commit message.
+	mapping map[common.Hash]*list.Element
+	// double linked list of commit messages
 	// to track the order commit messages were added in.
 	linkedList *list.List
 	capacity   int
 }
 
-type commitMessageMapData struct {
-	message *CommitMessage
-	// element contains a block hash value.
-	element *list.Element
-}
-
 // newCommitsTracker creates a new commit messages tracker
 // with the capacity specified.
 func newCommitsTracker(capacity int) commitsTracker {
 	return commitsTracker{
-		mapping:    make(map[common.Hash]commitMessageMapData, capacity),
+		mapping:    make(map[common.Hash]*list.Element, capacity),
 		linkedList: list.New(),
 		capacity:   capacity,
 	}
@@ -45,26 +38,21 @@ func newCommitsTracker(capacity int) commitsTracker {
 func (ct *commitsTracker) add(commitMessage *CommitMessage) {
 	blockHash := commitMessage.Vote.Hash
 
-	data, has := ct.mapping[blockHash]
+	listElement, has := ct.mapping[blockHash]
 	if has {
-		// commit already exists so override the commit for the block hash;
+		// commit already exists so override the commit message in the linked list;
 		// do not move the list element in the linked list to avoid
 		// someone re-sending the same commit message and going at the
 		// front of the list, hence erasing other possible valid commit messages
 		// in the tracker.
-		data.message = commitMessage
-		ct.mapping[blockHash] = data
+		listElement.Value = commitMessage
 		return
 	}
 
 	// add new block hash in tracker
 	ct.cleanup()
-	element := ct.linkedList.PushFront(blockHash)
-	data = commitMessageMapData{
-		message: commitMessage,
-		element: element,
-	}
-	ct.mapping[blockHash] = data
+	listElement = ct.linkedList.PushFront(commitMessage)
+	ct.mapping[blockHash] = listElement
 }
 
 // cleanup removes the oldest commit message from the tracker
@@ -79,40 +67,42 @@ func (ct *commitsTracker) cleanup() {
 	oldestElement := ct.linkedList.Back()
 	ct.linkedList.Remove(oldestElement)
 
-	oldestBlockHash := oldestElement.Value.(common.Hash)
+	oldestCommitMessage := oldestElement.Value.(*CommitMessage)
+	oldestBlockHash := oldestCommitMessage.Vote.Hash
 	delete(ct.mapping, oldestBlockHash)
 }
 
 // delete deletes all the vote messages for a particular
 // block hash from the vote messages tracker.
 func (ct *commitsTracker) delete(blockHash common.Hash) {
-	data, has := ct.mapping[blockHash]
+	listElement, has := ct.mapping[blockHash]
 	if !has {
 		return
 	}
 
-	ct.linkedList.Remove(data.element)
+	ct.linkedList.Remove(listElement)
 	delete(ct.mapping, blockHash)
 }
 
 // message returns a pointer to the
 // commit message for a particular block hash from
 // the tracker. It returns nil if the block hash
 // does not exist in the tracker
-func (ct *commitsTracker) message(
-	blockHash common.Hash) (message *CommitMessage) {
-	data, ok := ct.mapping[blockHash]
+func (ct *commitsTracker) message(blockHash common.Hash) (
+	message *CommitMessage) {
+	listElement, ok := ct.mapping[blockHash]
 	if !ok {
 		return nil
 	}
 
-	return data.message
+	return listElement.Value.(*CommitMessage)
 }
 
 // forEach runs the function `f` on each
 // commit message stored in the tracker.
 func (ct *commitsTracker) forEach(f func(message *CommitMessage)) {
 	for _, data := range ct.mapping {
-		f(data.message)
+		message := data.Value.(*CommitMessage)
+		f(message)
 	}
 }

lib/grandpa/commits_tracker_test.go

@@ -26,15 +26,15 @@ func buildCommitMessage(blockHash common.Hash) *CommitMessage {
 }
 
 func assertCommitsMapping(t *testing.T,
-	mapping map[common.Hash]commitMessageMapData,
+	mapping map[common.Hash]*list.Element,
 	expected map[common.Hash]*CommitMessage) {
 	t.Helper()
 
 	require.Len(t, mapping, len(expected), "mapping does not have the expected length")
 	for expectedBlockHash, expectedCommitMessage := range expected {
-		data, ok := mapping[expectedBlockHash]
+		listElement, ok := mapping[expectedBlockHash]
 		assert.Truef(t, ok, "block hash %s not found in mapping", expectedBlockHash)
-		assert.Equalf(t, expectedCommitMessage, data.message,
+		assert.Equalf(t, expectedCommitMessage, listElement.Value.(*CommitMessage),
 			"commit message for block hash %s is not as expected",
 			expectedBlockHash)
 	}
@@ -45,7 +45,7 @@ func Test_newCommitsTracker(t *testing.T) {
 
 	const capacity = 1
 	expected := commitsTracker{
-		mapping:    make(map[common.Hash]commitMessageMapData, capacity),
+		mapping:    make(map[common.Hash]*list.Element, capacity),
 		linkedList: list.New(),
 		capacity:   capacity,
 	}
@@ -197,17 +197,17 @@ func Test_commitsTracker_message(t *testing.T) {
 	}{
 		"non existing block hash": {
 			commitsTracker: &commitsTracker{
-				mapping: map[common.Hash]commitMessageMapData{
+				mapping: map[common.Hash]*list.Element{
 					{1}: {},
 				},
 			},
 			blockHash: common.Hash{2},
 		},
 		"existing block hash": {
 			commitsTracker: &commitsTracker{
-				mapping: map[common.Hash]commitMessageMapData{
+				mapping: map[common.Hash]*list.Element{
 					{1}: {
-						message: &CommitMessage{Round: 1},
+						Value: &CommitMessage{Round: 1},
 					},
 				},
 			},
@@ -272,7 +272,7 @@ func Benchmark_ForEachVsSlice(b *testing.B) {
 	getMessages := func(ct *commitsTracker) (messages []*CommitMessage) {
 		messages = make([]*CommitMessage, 0, len(ct.mapping))
 		for _, data := range ct.mapping {
-			messages = append(messages, data.message)
+			messages = append(messages, data.Value.(*CommitMessage))
 		}
 		return messages
 	}
@@ -285,15 +285,15 @@ func Benchmark_ForEachVsSlice(b *testing.B) {
 	const trackerSize = 10e4
 	makeSeededTracker := func() (ct *commitsTracker) {
 		ct = &commitsTracker{
-			mapping: make(map[common.Hash]commitMessageMapData),
+			mapping: make(map[common.Hash]*list.Element),
 		}
 		for i := 0; i < trackerSize; i++ {
 			hashBytes := make([]byte, 32)
 			_, _ = rand.Read(hashBytes)
 			var blockHash common.Hash
 			copy(blockHash[:], hashBytes)
-			ct.mapping[blockHash] = commitMessageMapData{
-				message: &CommitMessage{
+			ct.mapping[blockHash] = &list.Element{
+				Value: &CommitMessage{
 					Round: uint64(i),
 					SetID: uint64(i),
 				},