Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/vet: add check for non-pointers stored within sync.Pool #16311

Closed
rasky opened this issue Jul 9, 2016 · 11 comments

Comments

Projects
None yet
7 participants
@rasky
Copy link
Member

commented Jul 9, 2016

To avoid spurious allocations, sync.Pool should always be used with pointers. For instance, when storing buffers as slices, a pointer to slice should be used instead of the slice itself. This was brought up in http://golang.org/cl/24371 by @dvyukov.

Since the primary goal of sync.Pool is to reduce allocations, I think it might be worth adding a check to go vet about this. The check could be triggered on Put() calls in which the argument is not a pointer.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 9, 2016

I don't think we need to hold people's hands with low-level things.

@dominikh

This comment has been minimized.

Copy link
Member

commented Jul 9, 2016

I don't think we need to hold people's hands with low-level things.

But isn't that what vet already does? It barks at you for incorrect use of unsafe.Pointer/uintptr, for example. (Note that I don't agree with putting this check in vet as it's an optimization.)

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2016

I'm intrigued. This is almost certainly a bug, in that if you're going to the bother of using sync.Pool, you really care about allocations, in which case you really want to know that you're doing it wrong. I'm curious how many problems such a check would turn up. It has a few nice properties--approximately no false positives, results very likely to be of significant interest, etc.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 10, 2016

If you're going to the bother of using sync.Pool, you certainly already have a benchmark to show that your use of sync.Pool made things better. Your benchmark and existing profiling tools will quickly teach you about slice-in-interface allocating.

Plus, isn't cmd/vet about bugs, and not performance opportunities?

Does vet already warn about any other performance optimizations?

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2016

Your benchmark and existing profiling tools will quickly teach you about slice-in-interface allocating.

Good point. I'm convinced.

@rasky

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2016

FWIW, I manually checked a few code bases of mine and found one instance where I had this bug. I checked, and I had converted to sync.Pool a few things in a single commit and benchmarked the overall performance, so I didn't immediately realize the bug.

@dominikh

This comment has been minimized.

Copy link
Member

commented Jul 10, 2016

Plus, isn't cmd/vet about bugs, and not performance opportunities?

Right. vet only flags code that has a high chance of being wrong, as in producing wrong results or running risk of crashes, races and so on.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2016

As explained, this is not a suitable check to add to vet.

@robpike robpike closed this Jul 10, 2016

@dvyukov

This comment has been minimized.

Copy link
Member

commented Jul 11, 2016

@bradfitz do you know each and every allocation on the HTTP roundtrip path? If not, you can't say that there is no one that is easily fixable and should not be there in the first place (a perf bug).
FWIW the first usage of sync.Pool that I reviewed internally did cache slices by value. Author did very extensive benchmarking and concluded that sync.Pool does not give significant benefit.

@rasky

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2016

I suggest to update the documentation to mention this "requirement" prominently.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 11, 2016

@bradfitz do you know each and every allocation on the HTTP roundtrip path?

I did back during the cycle where I was working on that. I knew it because I had our tools which showed me all of them.

FWIW the first usage of sync.Pool that I reviewed internally did cache slices by value.

I've written and reviewed code with problems too. Profiling tools help.

@rasky, feel free to file a new documentation bug. This one is closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.