Perf: optimize fetching last file commit #58055
Conversation
Yes, which is why I added the bonus logic in In practice, if you actually care about paginating, I don't think you'd ever have a page size of 1 anyways, so I don't think this will ever be problematic. However, the real solution is to fix our graphql library so that we query the requested fields, making this whole dance unnecessary. |
|
Yeah, I didn't mean to block this PR, but returning |
|
Yeah you're right. I added some more comments. I hope we can solve the "we don't know what fields were requested" issue for real...but until then I think this is still worth the hack and slightly incorrect behavior. |
When fetching the information about the commit that modified a file, we use an ancestors query that is filtered to the file name. This is a paginated interface, so we always request n+1 items so we know whether there is a next page. However, the extra requested commit can actually be quite expensive to fetch, particularly in the relatively common case that a file has only been modified in one commit: the one that created it. In that case, we have to iterate over the entire history of the repository to look for a commit that doesn't exist to produce information about a next page that will never be fetched. The ideal solution here would be to only fetch n+1 if the hasNextPage field of the resolver is requested. Unfortunately, our GraphQL package still doesn't support querying for which fields are requested, which is a huge bummer. Instead, this takes a slightly hackier approach and assumes that if the number requested is 1, it's better to serve that quickly and claim we have a next page without knowing whether that page will contain anything.
When fetching the information about the commit that modified a file, we use an
ancestorsquery that is filtered to the file name. This is a paginated interface, so we always request n+1 items so we know whether there is a next page.However, the extra requested commit can actually be quite expensive to fetch, particularly in the relatively common case that a file has only been modified in one commit: the one that created it. In that case, we have to iterate over the entire history of the repository to look for a commit that doesn't exist to produce information about a next page that will never be fetched.
The ideal solution here would be to only fetch n+1 if the
hasNextPagefield of the resolver is requested. Unfortunately, our GraphQL package still doesn't support querying for which fields are requested, which is a huge bummer.Instead, this takes a slightly hackier approach and assumes that if the number requested is 1, it's better to serve that quickly and claim we have a next page without knowing whether that page will contain anything.
Test plan
Manually tested the the trace of the
FetchOwnersAndHistoryGraphQL request. For a file with only one commit, the span representing the git command shrunk from 300ms to 70ms, and there no longer a visible loading indicator.