Optimization opportunity to reuse service struct allocations in github.Client

[mdempsky]

All of github.Client's service fields point to objects with identical underlying struct types. Currently they're each separately heap allocated, but they could be made to reuse the same memory. For example, add a field to github.Client:

       self       struct{ client *Client }

And then change github.NewClient to initialize the service fields as so:

       c.self.client = c
       c.Activity = (*ActivityService)(&c.self)
       c.Authorizations = (*AuthorizationsService)(&c.self)
       c.Gists = (*GistsService)(&c.self)
       ...

Just a random observation while looking at the API/implementation. Feel free to close if you don't think this optimization is worthwhile.

[gmlewis]

Interesting. This saves roughly (14-1)*8=104 bytes on the heap on a 64-bit system if my math is correct. The initialization code is a bit uglier but otherwise there should be no changes to the users of the package. I'm on the fence as to whether or not this is worth doing. I'll wait to hear from others.
Whichever way it goes, thanks for the suggestion, @mdempsky.

[mdempsky]

Math checks out to me. FWIW, the savings are the same on 32-bit systems, because heap allocations for variables containing pointers are always at least 8 bytes independent of pointer size.

[willnorris]

huh, certainly a clever optimization. Like Glenn said, it does make the initializion code a little uglier, but that's not really code that anyone touches except to add a new service. I could really go either way on making the change or not. @shurcooL: you have an opinion either way?

[dmitshur]

That's a neat idea. I didn't realize you could do this. It is valid though.

@shurcooL: you have an opinion either way?

I agree with everything said and disagree with nothing. This sentence summarizes it best:

it does make the initialization code a little uglier, but that's not really code that anyone touches except to add a new service.

I'll add one thought to that.

I think it's fantastic how this library serves a very well defined and useful need (a GitHub API Go client). That allows it to be used by many, and so even marginal improvements can have a large positive impact.

One my personal principles is to never say no when someone wants to make something better, no matter how little or marginal the improvement is. As long as it's a net positive improvement, I think it's a bad idea to not let a motivated person from making that contribution, just because it's too upsetting to be on the other side (feeling excited, "here's a small improvement I want to make! But is it going to be rejected because it's deemed "too small" and not worth the time?").

We've had some cool enhancements contributed, both really significant like #317 (/ht @FiloSottile) and pretty insignificant like #347 (shameless plug). I think all should be welcome, if there's someone motivated who wants to contribute and the result is better.

I see this idea in similar vein. Yes, the potential improvement is minor, but once done, all users will benefit. It would make internal implementation details that users don't look at a little messier, and prevent new service structs from having extra fields inside... But I doubt we'd ever want to change that anyway, and we if do, it's easy to revert this optimization.

So to summarize, I think this optimization idea is neat/cool, and I learned something from it. I don't think it's significant enough that I would work on it. But if someone (@mdempsky?) wants to make a PR, and it works out well, I'm very happy to review and accept it!

[gmlewis]

@shurcooL - I really like that philosophy. SGTM. 👍

If anyone wants to take this one, feel free. If not, I'm happy to take it. I'll give @mdempsky a couple days to decide. (I have a personal goal of getting to zero PRs and zero Issues... I'm not sure it can truly be accomplished, but I think we've made awesome progress as a team in the last month or two.)

[mdempsky]

Cool, I'll upload a PR later today then.