disintegration / imaging

Imaging is a simple image processing package for Go
MIT License
5.3k stars 443 forks source link

Support limiting the number of parallel processing goroutines #123

Closed marianrh closed 4 years ago

marianrh commented 4 years ago

Hi,

thanks a lot for your work and this great library!

I have a suggestion for limiting the number of parallel processing goroutines.

The reason for the proposal is my use case, where I'm using the Resize function in an image processing server. The server is accessed simultaneously by multiple clients. Beside resizing images, the server also has to perform other tasks. Therefore, it's problematic that a single Resize runs runtime.GOMAXPROCS goroutines via the parallel function.

This pull request proposes to add an exported global variable that can be used to limit the number of goroutines.

Best regards, Marian

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-0.1%) to 99.888% when pulling 3ac28e9eb4ba54415c46a6bf8402b228c14cb4ef on marianrh:master into 879073f2332fabf5a5faa93986ed41598dcf0b62 on disintegration:master.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 5232859032443cb6ea1f88152cf6aad74e2ada08 on marianrh:master into 879073f2332fabf5a5faa93986ed41598dcf0b62 on disintegration:master.

disintegration commented 4 years ago

Hello, Thank you for the contribution!

Generally I try to avoid using global variables as much as possible, but I guess we don't have a lot of options on how to add such a limit given the current package design.

I believe the better way to set the limit would be using a function. In this case the variable can be unexported and accessed using atomics, e.g.:

var maxProcs int64

// ...

func SetMaxProcs(value int) {
    atomic.StoreInt64(&maxProcs, int64(value))
}

// ...

limit := int(atomic.LoadInt64(&maxProcs))
if procs > limit && limit > 0 {
    procs = limit
}

What do you think?

Regarding the documentation string, I think it would be better to change the phrase parallel processing subroutines to concurrent processing goroutines.

marianrh commented 4 years ago

Of course, you're right that this has to be set and read atomically. I've done the changes as you suggested.

I added a comment to SetMaxProcs. I guess we could remove the comment for maxProcs now that it's not exported anymore, what do you think?

disintegration commented 4 years ago

I guess we could remove the comment for maxProcs now that it's not exported anymore, what do you think?

Sure, we can remove it now.

marianrh commented 4 years ago

Sure, we can remove it now.

Done.

disintegration commented 4 years ago

Thank you!