-
Notifications
You must be signed in to change notification settings - Fork 369
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
Adds concurrency to hydration requests #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! is this in response to any particular performance complaints in particular?
pkg/osv/osv.go
Outdated
go func(resp *BatchedResponse) { | ||
for batchIdx, response := range resp.Results { | ||
for resultIdx, vuln := range response.Vulns { | ||
rateLimiter <- struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we use https://pkg.go.dev/golang.org/x/sync/semaphore#example-package-WorkerPool instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did see that package, but since semaphores are under the /x/ experimental package I'm not sure whether we should use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fair game to use it. There's a lot of prominent packages using it already: https://deps.dev/go/golang.org%2Fx%2Fsync/v0.1.0/dependents
Not any issue in particular, I just realized the sequential calls are a major part of the run time when looking at implementing #303 . |
@another-rex are we ready to merge? |
An attempt at concurrency for hydration requests. I'm not sure if this is the most concise way to do so, it seems pretty long for what should be a simple concurrency logic.