vbauerster / mpb

multi progress bar for Go cli applications
The Unlicense
2.29k stars 123 forks source link

Ineffective breaks in heap_manager.go #128

Closed egonelbre closed 1 year ago

egonelbre commented 1 year ago

I was looking through the codebase and when running staticcheck it lists these issues:

heap_manager.go:81:6: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
heap_manager.go:91:6: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)

I'm not sure whether you want to return or just exit the bHeap handling.


Other things I noticed that aren't errors per se, but looked unusual to me:

https://github.com/vbauerster/mpb/blob/d8400e981502dafd3884e25d826a01ae659a74c2/decor/eta.go#L203

time.Duration has Truncate func: https://pkg.go.dev/time#Duration.Truncate

https://github.com/vbauerster/mpb/blob/d8400e981502dafd3884e25d826a01ae659a74c2/decor/pool.go

using a var buffer [64]byte should get you most of the way and avoid scenarios where you accidentally pool large []byte values for a long time.

vbauerster commented 1 year ago

I was looking through the codebase and when running staticcheck it lists these issues:

Yep, staticcheck's suggestion is right, despite current behavior doesn't lead to any bugs.

Other things I noticed that aren't errors per se, but looked unusual to me:

Thankfully I left a comment strip off nanoseconds. I agree that Truncate func is better option here, thanks for suggestion.

using a var buffer [64]byte should get you most of the way and avoid scenarios where you accidentally pool large []byte values for a long time.

It's used to feed strconv.AppendFloat with dst []byte param. Did you mean to increase cap to 64 like b := make([]byte, 0, 64) or using array [64]byte instead of slice? If latter then how can it help/improve?

egonelbre commented 1 year ago

I meant the var buffer [64]byte version. Take a look at https://go.dev/play/p/0ig2yQgA3gg.

Benchmark results:

name              time/op
Append_Pooled-32  214ns ± 5%
Append_Stack-32   202ns ± 1%
Append_Alloc-32   201ns ± 0%

Also, avoiding the pool is also going to less code and easier to reason about.

While reviewing the code, I realized that that it's always putting back the small slice, so there's no problem with holding on to a potentially large slices.

vbauerster commented 1 year ago

Addressed in v8.4.0.