Predict *RateLimitError, return immediately without network call (9700x speedup when rate limit exceeded).

[dmitshur]

This (minor) optimization is possible because the client keeps track of the rate limits from previous API calls, and GitHub's rate limiting model is known in advance, and is predictable.

In situations where it's possible to predict that the network API call will result in a *RateLimitError, simply return it immediately instead of making the network API call to GitHub's servers. This is both faster for the client (by around 9700 times in my benchmarks) because it avoids the network round-trip latency, and better for GitHub's servers that won't need to needlessly reject network requests.

Note that this optimization only kicks in when the client is already exceeding the rate limit and it hasn't been reset yet. It has no effect during most normal usage when rate limit is not exceeded.

Benchmarks

I wrote a small benchmark (it assumes you've already exhausted the GitHub API rate limit before you begin):

func BenchmarkClientDo_rateLimitError(b *testing.B) {
    gh := github.NewClient(nil)
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        _, _, err := gh.Repositories.Get("shurcooL", "vfsgen")
        if _, ok := err.(*github.RateLimitError); !ok {
            log.Println("no *github.RateLimitError error, but expected it")
        }
    }
}

Results before this PR (with a latency to api.github.com of around 85 ms):

$ go test -bench=. -benchmem
testing: warning: no tests to run
PASS
BenchmarkClientDo_rateLimitError-8        20      97057828 ns/op       55922 B/op        126 allocs/op
ok          2.609s

Results after this PR:

go test -bench=. -benchmem
testing: warning: no tests to run
PASS
BenchmarkClientDo_rateLimitError-8    200000         10005 ns/op        2763 B/op         34 allocs/op
ok          3.576s

In this limited scenario, it's a speedup by 9700 times.

Helps #152 and #153.

[gmlewis]

Curious... why do you list default: first? I don't remember seeing it that way before.

[dmitshur]

It's a style I've iterated towards after using switch statements more for multiplexing across multiple (usually non-overlapping) branching decisions in Go. I prioritize the order of cases, returning coreCategory and searchCategory, to be more consistent with how they're defined, over the conditions that lead to those return values.

I read it as:

For all paths, return coreCategory, except when it has /search/ prefix, then return searchCategory.

I personally like it because I think the cases themselves are more important than the conditions to trigger them, but I know this is a less common style. If people feel this is worse, I can revert to a more traditional style of sorting by conditions (putting default last).

[gmlewis]

For other people reading, I want to note that the order of case expressions in Go is important... they are evaluated left-to-right and top-to-bottom and the first one that matches wins (except for default which wins when no others match).
https://golang.org/ref/spec#Switch_statements

Keeping it this way is fine with me.

[dmitshur]

@gmlewis, I've addressed your current feedback, PTAL.

[gmlewis]

Thanks, @shurcooL! LGTM 👍
Awaiting review by @willnorris

[dmitshur]

@willnorris Should I take the longer time to review to mean you're not as confident about this change and not sure how to respond to it, or have you simply not gotten around to it yet?

This change, at a high level, is very much in line with what you discussed/proposed in these two comments:

#152 (comment)
#153 (comment)

I think the high level design is solid.

About the implementation, I actually took a lot of time to come up with how to achieve the desired behavior, because I knew there were more than 1 sets of rate limits, depending on the route. But I think the final solution I came up with is pretty decent and works. But of course I welcome any potential suggestions for how to make the implementation even cleaner.

Let me know if I can do anything to help here.

[willnorris]

just a case of me being out of town when the PR came in, and then busy with other things. Looking at it now.

[willnorris]

this seems potentially problematic, since if a single client is being used for both search and non-search calls, the "most recent category" could get switched out from under you by another go routine before you have a chance to call Rate. What if Rate always returns the core rate (like it did before), and we provide a different mechanism to access to other rates? Or alternatively, we just instruct callers to pull that rate off the response? checkRateLimitBeforeDo would still handle both categories as it does currently.

[willnorris]

actually yeah, we don't really need to provide another mechanism... we already instruct users to call client.RateLimits for the most up to date values. That should be sufficient, no?

[dmitshur]

if a single client is being used for both search and non-search calls, the "most recent category" could get switched out from under you by another go routine before you have a chance to call Rate.

If I'm not mistaken, that cannot happen. Both rateLimits and mostRecent are in a critical section protected by rateMu mutex. That mutex is very important to prevent this problem. Is that not the case?

[dmitshur]

Oh, I think I see what you meant. You weren't referring to a race condition in the code, but a higher-level "logical" race condition. E.g., a client makes a core category API call and then calls Rate expecting to see rate limit for that category, but may instead get rate limit for the search category.

I agree that's potentially bad and should be considered. I'll think about this and get back to you later.

One observation is that each API call already returns a Response which contains the rate limit from that very API call. So there's really no need for client to call Rate. You mentioned that in "we just instruct callers to pull that rate off the response".

[dmitshur]

What if Rate always returns the core rate (like it did before)

Now that I think about it more, are you sure that's what Rate currently does? Doesn't it already always return the latest rate limit from either of two categories, whichever was called last? In other words, this PR might be currently preserving previous behavior.

If you're sure that the current Rate only contains core category rate limits, can you show me where that happens and what prevents a call to search from updating rate?

It looks to me that search API calls would update it also. See here:

• go-github/github/github.go

  Lines 350 to 352 in 81ea1e5

  	c.rateMu.Lock()
  	c.rate = response.Rate
  	c.rateMu.Unlock()

• go-github/github/search.go

  Line 157 in 81ea1e5

  return s.client.Do(req, result)

[willnorris]

yep, looks like you're right... search is already updating the client rate, which leads to this same problem. Maybe we should just deprecate this method entirely since it's likely to cause problems, and just instruct people to use the rate limit on the response, or to call client.RateLimits? What do you think?

[dmitshur]

Maybe we should just deprecate this method entirely since it's likely to cause problems, and just instruct people to use the rate limit on the response, or to call client.RateLimits? What do you think?

I am absolutely in favor of doing that, given those 2 better alternatives exist, and the current behavior is pretty unreliable (it can give you core or search rate limit, effectively randomly).

[willnorris]

Sounds good. Could you go ahead and update the doc for this method then as part of this PR?

[dmitshur]

Done in 13c2701. (I've also squashed the previous commits that were implementing code review suggestions to have a clean, logical history.)

[willnorris]

looks great, thanks!

[dmitshur]

Awesome, thanks! It's not every day one gets a chance to submit a PR with a 9700x perf improvement. :P