dont create a new ticker each loop #1180
Conversation
whyrusleeping
added
kind/bug
|
Looks good, no ramp after 9h running with this patch (and the oom fix).
|
|
@okket thank you for verifying! |
|
|
||
| for { | ||
| select { | ||
| case <-time.Tick(10 * time.Second): |
There was a problem hiding this comment.
interesting. should grep the codebase and make sure this doesn't happen elsewhere. Was it just not getting gc-ed? or is there some releasing we need to do?
There was a problem hiding this comment.
I searched the codebase for any other similar mistakes and found none.
time.Tick creates a channel intended to be used repeatedly, it does some magic runtime stuff and inserts a system timer callback to send on the channel, so it doesnt show up as a goroutine.
There was a problem hiding this comment.
We should call http://golang.org/pkg/time/#Ticker.Stop everywhere
dont create a new ticker each loop
|
thanks @okket @whyrusleeping -- this is great |
| @@ -182,10 +182,11 @@ func (bs *Bitswap) rebroadcastWorker(parent context.Context) { | |||
| defer cancel() | |||
|
|
|||
| broadcastSignal := time.After(rebroadcastDelay.Get()) | |||
| tick := time.Tick(10 * time.Second) | |||
There was a problem hiding this comment.
maybe:
tick := time.NewTicker(10 * time.Second)
defer tick.Stop()There was a problem hiding this comment.
Eh, not really needed. This is a single goroutine that lasts the entire lifetime of the executable.
There was a problem hiding this comment.
Today.
And not if we start many nodes and tear them down in one binary. Found a good bug. We should finish the fix. Do we lose anything by adding a stop?
—
Sent from Mailbox
On Sat, May 2, 2015 at 1:50 PM, Jeromy Johnson notifications@github.com
wrote:
@@ -182,10 +182,11 @@ func (bs *Bitswap) rebroadcastWorker(parent context.Context) {
defer cancel()broadcastSignal := time.After(rebroadcastDelay.Get())
- tick := time.Tick(10 * time.Second)
Eh, not really needed. This is a single goroutine that lasts the entire lifetime of the executable.
Reply to this email directly or view it on GitHub:
https://github.com/ipfs/go-ipfs/pull/1180/files#r29550583
|
Looks like 9h were not enough, the ramp is back:
Are any of there potential candidates? |
|
@okket thats weird....is the left side of that graph the run you did to verify the bugfix for me? did you bring the daemon down in between? |
|
I thought I double checked everything, but the timestamp on the binary looks suspicious. I'll try another build and run. Maybe I did something wrong (likely). |
|
Looks like I messed up yesterday. After 14h with https://github.com/ipfs/go-ipfs/tree/fab5e91a93fa83594796478fb7864c8e5b5f0000 no ramp in sight:
|
|
@okket i'm glad that you were wrong this time, lol 👍 |
|
Yeah, sorry for the false alarm. Good that this is finally fixed, thank you for your hard work. Now I can let the node run without getting annoying graphs 😀 |
|
@okket glad to hear it 😄 Definitely let me know if you experience any other weird issues! I wanna see how long we can keep a node online |
|
Yay, no ramp! 1.5k ctx switches per second prob still something to reduce. — On Mon, May 4, 2015 at 5:42 AM, Jeromy Johnson notifications@github.com
|
Well, it seems that my perf kick is turning up more bugs than perf improvements... sigh
But this one is a pretty ugly and subtle one which i believe is the cause of #1036
A ticker starts a timer that continuously sends on the given channel, every loop, a new one was being created.
I wrote the following program and let it run for a few minutes, watching the number of context switches start out around an idle 600/s, slowly rise to over 10k/s before i killed the program, and the number of context switches resumed to the normal 600/s.
I searched the codebase for any other similar mistakes and found none.