Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fragmentation of excessive broadcast packets #761

Open
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nkbai
Copy link

commented Mar 17, 2019

  1. Fragmentation of excessive broadcast packets
  2. Makefile work problem on Linux platform

nkbai and others added some commits Feb 23, 2019

@nkbai nkbai requested a review from iotexproject/protocol-team as a code owner Mar 17, 2019

@zjshen14
Copy link
Contributor

left a comment

@nkbai thanks for the contribution. The fragmentation approach looks fine overall. Some comments about the details:

  1. Missing proto file change?
  2. Can we add a config to optionally enable fragmentation?
  3. We only apply it to broadcast. We plan to work on unicast on another PR?
  4. Consider make whole broadcast message, and fragment be different types to avoid confusion
  5. unit tests are recommended for broadcastHelper
@@ -54,6 +54,8 @@ COV_HTML := coverage.html

LINT_LOG := lint.log

#blslib finding
MY_LD_LIBRARY_PATH=$(LD_LIBRARY_PATH):$(PWD)/crypto/lib:$(PWD)/crypto/lib/blslib ./bin/$(BUILD_TARGET_SERVER)

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

$(PWD)/crypto/lib/blslib is no longer necessary

This comment has been minimized.

Copy link
@nkbai

nkbai Mar 20, 2019

Author
  1. Unicast does not require fragmentation. For unicast, packets larger than 1M can be sent and received normally.
export LD_LIBRARY_PATH=$(LD_LIBRARY_PATH):$(PWD)/crypto/lib
./bin/$(BUILD_TARGET_SERVER) -plugin=gateway
$(GOBUILD) -o ./bin/$(BUILD_TARGET_SERVER) -v ./$(BUILD_TARGET_SERVER)
LD_LIBRARY_PATH=$(MY_LD_LIBRARY_PATH) ./bin/$(BUILD_TARGET_SERVER) -config-path=e2etest/config_local_delegate.yaml

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

-config-path=e2etest/config_local_delegate.yaml is no longer necessary

"sync"
"testing"
"time"

pubsub "github.com/libp2p/go-libp2p-pubsub"

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

No empty line in between

}
//remove message if too many items
if len(h.bhs) > maxItemForBroadcast {
bh := heap.Remove(h, 0) //移除第一个,时间最久的那个

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

In english, please

for len(h.bhs) > 0 {
bh := h.bhs[0]
//remove item that is arrived at 1 minute ago
if bh.t.After(time.Now().Add(0 - time.Minute)) {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

define 1 minute up front as the const?

}

//AddMessage add a new piece of message
func (h *broadcastHelperHeap) AddMessage(msg *iotexrpc.BroadcastMsg) *iotexrpc.BroadcastMsg {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

Not thread safe, but its okay for broadcast, because pubsub seems to synchronize the message handling.

Unicast seems not.

t: time.Now(),
}
if !msg.HasMore {
bh.maxIndex = int(msg.IndexOfPiece)

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

what if out of order? 3 arrives earlier than 1. When receiving 1, bh.maxIndex will be set to 1 then?

This comment has been minimized.

Copy link
@nkbai

nkbai Mar 20, 2019

Author

only the last one's HasMore is true,

bh.broadcast.MsgBody = append(bh.broadcast.MsgBody, bh.bodies[i]...)
}
delete(h.id2BroadcastHelper, bh.broadcast.MessageId)
return bh.broadcast

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

how to remove from the heap, which is not necessarily to be heap head

bh.maxIndex = int(msg.IndexOfPiece)
}
bh.t = time.Now()
sort.Sort(h)

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Mar 18, 2019

Contributor

sort after potentially removing the the complete message?

@codecov

This comment has been minimized.

Copy link

commented Mar 24, 2019

Codecov Report

Merging #761 into master will increase coverage by 0.4%.
The diff coverage is 73%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #761     +/-   ##
=========================================
+ Coverage   59.62%   60.02%   +0.4%     
=========================================
  Files         133      134      +1     
  Lines       11486    11582     +96     
=========================================
+ Hits         6848     6952    +104     
+ Misses       3754     3743     -11     
- Partials      884      887      +3
Impacted Files Coverage Δ
p2p/broadcasthelper.go 66.66% <66.66%> (ø)
p2p/agent.go 66.66% <83.78%> (+3.6%) ⬆️
consensus/scheme/rolldpos/rolldposctx.go 75.11% <0%> (-0.46%) ⬇️
action/protocol/poll/protocol.go 20.27% <0%> (+0.09%) ⬆️
config/config.go 53.04% <0%> (+1.69%) ⬆️
state/factory/factory.go 72.83% <0%> (+3.42%) ⬆️
state/factory/statedb.go 75.88% <0%> (+4.14%) ⬆️
state/factory/workingset.go 81.66% <0%> (+8.33%) ⬆️
state/factory/statetx.go 89.28% <0%> (+11.9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a8dda7...8479e9d. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Mar 24, 2019

Codecov Report

Merging #761 into master will increase coverage by 0.4%.
The diff coverage is 73%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #761     +/-   ##
=========================================
+ Coverage   59.62%   60.02%   +0.4%     
=========================================
  Files         133      134      +1     
  Lines       11486    11582     +96     
=========================================
+ Hits         6848     6952    +104     
+ Misses       3754     3743     -11     
- Partials      884      887      +3
Impacted Files Coverage Δ
p2p/broadcasthelper.go 66.66% <66.66%> (ø)
p2p/agent.go 66.66% <83.78%> (+3.6%) ⬆️
consensus/scheme/rolldpos/rolldposctx.go 75.11% <0%> (-0.46%) ⬇️
action/protocol/poll/protocol.go 20.27% <0%> (+0.09%) ⬆️
config/config.go 53.04% <0%> (+1.69%) ⬆️
state/factory/factory.go 72.83% <0%> (+3.42%) ⬆️
state/factory/statedb.go 75.88% <0%> (+4.14%) ⬆️
state/factory/workingset.go 81.66% <0%> (+8.33%) ⬆️
state/factory/statetx.go 89.28% <0%> (+11.9%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a8dda7...8479e9d. Read the comment docs.

counts[idx]++
} else {
counts[idx] = 1
bh := func(name string) HandleBroadcastInbound {

This comment has been minimized.

Copy link
@golangcibot

golangcibot Mar 24, 2019

testBroadcastNumber$2 - name is unused (from unparam)

}
heap.Remove(h, index)
return bh.broadcast
} else {

This comment has been minimized.

Copy link
@golangcibot

golangcibot Mar 24, 2019

if block ends with a return statement, so drop this else and outdent its block (from golint)

PeerId: "foo",
MessageId: "a",
}
var frags []interface{}

This comment has been minimized.

Copy link
@golangcibot

golangcibot Mar 24, 2019

Consider preallocating frags (from prealloc)

msg2 := msg
msg2.IndexOfFrag = 2
msg2.HasMore = false
result = h.AddMessage(&msg2)

This comment has been minimized.

Copy link
@golangcibot

golangcibot Mar 24, 2019

SA4006: this value of result is never used (from staticcheck)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.