Avoid unnecessary FooService allocations #390
Conversation
| @@ -107,6 +107,7 @@ type Client struct { | |||
| rateMu sync.Mutex | |||
| rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls. | |||
| mostRecent rateLimitCategory | |||
| common service | |||
There was a problem hiding this comment.
I have a style suggestion.
rateMu sync.Mutex
rateLimits [categories]Rate
mostRecent rateLimitCategoryHere, rateMu is a mutex hat. Like a hat, it sits on top of the variables that it protects.
So, without needing to actually write the comment, the above is implicitly understood to be equivalent to:
// rateMu protects rateLimits and mostRecent fields.
rateMu sync.Mutex
rateLimits [categories]Rate
mostRecent rateLimitCategoryWhen adding a new, unrelated field that isn't protected by rateMu, don't do this:
rateMu sync.Mutex
rateLimits [categories]Rate
mostRecent rateLimitCategory
+common serviceInstead, do this:
rateMu sync.Mutex
rateLimits [categories]Rate
mostRecent rateLimitCategory
+
+common serviceHow does that sound?
I can't remember where I first saw this pattern, but it might've been @davecheney. I really like it and recommend not breaking it. (I'll try to fix up the clientMu mutex on top to use this pattern too.)
There was a problem hiding this comment.
How does that sound?
SGTM, done.
| c.Users = (*UsersService)(&c.common) | ||
| c.Licenses = (*LicensesService)(&c.common) | ||
| c.Migrations = (*MigrationService)(&c.common) | ||
| c.Reactions = (*ReactionsService)(&c.common) |
There was a problem hiding this comment.
could you go ahead and sort these as well? They were always supposed to be, but it looks like the last few got added to the bottom.
|
See comments, but otherwise I think this looks great! LGTM. My only ask is when merging this, squash into 1 commit so the relevant commit message pointing to more details/context is fewer git-blame hops away. |
|
made the minor changes suggested above, squashed and merged. Thanks @mdempsky! |
This change makes the code more consistent with the rateMu below. This is a followup to #390 (comment). Closes #396. Change-Id: I0fc8b280911f9171fb4d8e3fa60cf6d53b64a65b
This change makes the code more consistent with the rateMu below. This is a followup to google#390 (comment). Closes google#396. Change-Id: I0fc8b280911f9171fb4d8e3fa60cf6d53b64a65b
This change makes the code more consistent with the rateMu below. This is a followup to google/go-github#390 (comment). Closes #396. Change-Id: I0fc8b280911f9171fb4d8e3fa60cf6d53b64a65b
Fixes #389.