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

Limit concurrency #1710

Merged
merged 15 commits into from
Oct 8, 2019
Merged

Conversation

harmony-ek
Copy link
Contributor

@harmony-ek harmony-ek commented Oct 8, 2019

Do not spawn unbounded number of goroutines for parallel message processing. Instead, use a worker pool of 32 goroutines sharing the same intake queue. Log ("rx overrun") and tail-drop messages if the queue is full.

Tests

Local test pass; 100% coverage of the newly added core code (./msgq):

git checkout harmony-one/master
go test -count=1 -cover ./... > coverage-before.txt
git checkout limit_concurrency 
go test -count=1 -cover ./... > coverage-after.txt
sed -i.bak -E 's/  [0-9]+(\.[0-9]*)?s      /       /' coverage-*.txt
diff -U0 coverage-before.txt coverage-after.txt
--- coverage-before.txt 2019-10-08 16:13:42.000000000 -0700
+++ coverage-after.txt  2019-10-08 16:13:42.000000000 -0700
@@ -86 +86,2 @@
-ok     github.com/harmony-one/harmony/node     coverage: 15.4% of statements
+ok     github.com/harmony-one/harmony/msgq     coverage: 100.0% of statements
+ok     github.com/harmony-one/harmony/node     coverage: 14.3% of statements

TODO

  • Add UTs

@harmony-ek harmony-ek requested review from fxfactorial, LeoHChen, mikedoan and a team October 8, 2019 18:00
@harmony-ek harmony-ek self-assigned this Oct 8, 2019
node/node.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
@fxfactorial
Copy link
Contributor

Wondering if we should have go routine wrapper, like withMaxConcurrency(max int, caller_provide_cb)

@harmony-ek
Copy link
Contributor Author

Wondering if we should have go routine wrapper, like withMaxConcurrency(max int, caller_provide_cb)

Thought about doing so, but didn't go for it because it would lose type safety: the intake channel's type must be made generic (chan interface{}) and the recipient would have to type-assert it from interface{} which opens a possibility of a panic when misused/abused (msg := item.(incomingMessage) where the received item is not an incomingMessage), and even with a type check (if msg, ok := item.(incomingMessage); !ok { ... }) it creates a spurious branch that hurts readability.

This is a cost-benefit trade off. I'd say factor the logic out if we do this maybe the third time.

(O generics, where art thou…?)

@rlan35
Copy link
Contributor

rlan35 commented Oct 8, 2019

LGTM, [sidenote, it would be better if it support prioritization scheduling of the messages. Consensus should take higher priority]

@harmony-ek harmony-ek merged commit 1d56344 into harmony-one:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants