perf(api): Improve asynchronous behaviour #3638
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelkedar
requested review from
another-rex,
cuixq,
hogo6002,
jess-lowe and
oliverchang
another-rex
previously approved these changes
Jul 7, 2025
| return vulnerability_pb2.Vulnerability(id=self.id(), modified=modified) | ||
|
|
||
| def to_vulnerability(self, | ||
| include_source=False, |
Contributor
There was a problem hiding this comment.
Maybe something better suited to a followup PR, but I would suggest removing all these flags and force callers to use the async version if they want to include stuff.
| return vulnerability_pb2.Vulnerability(id=self.id(), modified=modified) | ||
|
|
||
| @ndb.tasklet | ||
| def to_vulnerability_minimal_async(self, |
Contributor
There was a problem hiding this comment.
Should we also be converting to_vulnerability_minimal to just call the async version then blocking so they both return the same data?
Member
Author
There was a problem hiding this comment.
I might just leave it for now, since it's kind of the opposite of your other comment.
(I don't think to_vulnerability_minimal() is even used anymore anyway)
another-rex
approved these changes
Jul 7, 2025
hogo6002
approved these changes
Jul 7, 2025
michaelkedar
added a commit
that referenced
this pull request
Jul 8, 2025
Bump the max concurrent requests from 5 to 10 for the API backend, to help with container instance scaling. This is not for the batch query backend, which still is just 1. Hopefully this won't have a notable performance impact. I don't really want to change this and #3638 at the same time, since I want to be able to measure the individual impacts of both. We could probably push this out with the release this week, and let #3638 stage until next week.
jess-lowe
approved these changes
Jul 8, 2025
michaelkedar
added a commit
to michaelkedar/osv.dev
that referenced
this pull request
Jul 10, 2025
Change all(?) the ndb calls used throughout the API handlers to use the async versions, and rewrote a few functions to not block on the completion of futures that it doesn't need to (particularly, converting the Bug to a proto, and populating the related/alias/upstream fields of those protos). Testing on my private instance, this should be a 3-5x speedup for queries with lots of vulnerabilities. I've also made the batch queries properly set the modified time from the alias/upstream group (which might actually be a tiny performance loss) to ensure the modified time is always the same for the batch query and the get by ID. I've added `@ndb.synctasklet` to the rpc handlers to let us use `yield` instead of `.result()`, just to discourage usage of it (using `.result()` in a tasklet can cause stack overflows)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change all(?) the ndb calls used throughout the API handlers to use the async versions, and rewrote a few functions to not block on the completion of futures that it doesn't need to (particularly, converting the Bug to a proto, and populating the related/alias/upstream fields of those protos).
Testing on my private instance, this should be a 3-5x speedup for queries with lots of vulnerabilities.
I've also made the batch queries properly set the modified time from the alias/upstream group (which might actually be a tiny performance loss) to ensure the modified time is always the same for the batch query and the get by ID.
I've added
@ndb.synctaskletto the rpc handlers to let us useyieldinstead of.result(), just to discourage usage of it (using.result()in a tasklet can cause stack overflows)