.*: replace linters with golangci-lint

[GiedriusS]

Replace all of our current linters with a meta linter which includes all
of the linters that we've used before and more.

Fixed detected problems:

• api/v1/query{,range} now properly adds timeout on top of the context
• Added missing error checking
• pkg/pool now uses pointers to slices. AIUI this means that we are now holding references to slices for a smaller amount of time and the deallocation happens faster (it works more efficiently) thus the alloc counter doesn't make much sense any more. I have removed it.

These two simple programs show that indeed it is more efficient to store ptrs-to-slices in a sync.Pool:
https://play.golang.org/p/nWaPErUmKM9
https://play.golang.org/p/7tSpZYMhjNa

Plus: http://staticcheck.io/docs/checks#SA6002

• Other minor cleanups of dead code and now we follow best practices

Verification: go test, played around with Thanos Query w/ these changes - everything seems to work like expected.

[bwplotka]

LGTM, let's do this!

[bwplotka]

We need some fixes, but I think we also need to make sure there is exclude list for errcheck things. We used to have a list in https://github.com/improbable-eng/thanos/blob/b4d233e6fe904c52a22948afea3c3002d894e5df/.errcheck_excludes.txt#L1 but seems like now it is ignored by this change?

[GiedriusS]

We need some fixes, but I think we also need to make sure there is exclude list for errcheck things. We used to have a list in

https://github.com/improbable-eng/thanos/blob/b4d233e6fe904c52a22948afea3c3002d894e5df/.errcheck_excludes.txt#L1
but seems like now it is ignored by this change?

Yes, will do. Will bring it back. This is just a very initial version for testing how it works in general.

[GiedriusS]

I will add InstrumentHandler() deprecation warnings into an ignore file for now and then I'll make an issue for it - we need to definitely migrate off of them because they have been completely removed recently. Rationale: I don't want to make this PR too big.

[bwplotka]

@adrien-f is it similar to your work? @GiedriusS is it rdy for review?

[GiedriusS]

Not yet, got a bit too trigger happy. Still need to fix out why the tests keep failing.

[povilasv]

some nits and questions around pointer to byte slice

[bwplotka]

Thanks! Some comments though.

Please keep in mind that it's fine to add lots of exceptions from linter and fix in another PRs.

Some changes might actually change semantics, so the more we will split PR, better TBH. @GiedriusS

[brancz]

lgtm 👍 very nice

[brancz]

If we don't already, let's open an issue for this.

[GiedriusS]

#1304 👍 good catch

[bwplotka]

Amazing, extremely important but tedious work that has to be done.

Thank You @GiedriusS LGTM! 💪

[bwplotka]

I think @povilasv had quesitons around byte slice. I think if it helps allocationwise - I don't see harm. I would merge this.

[povilasv]

I think @povilasv had quesitons around byte slice. I think if it helps allocationwise - I don't see harm. I would merge this.

Not really, so the slice header is sort of:

type sliceHeader struct {
ptr *Elem
len int
cap int
}

copying 2 ints is a trivial op, so I still don't like the pointers to slices.

We went from:

p.buckets[i].Put(b[:0])

to

*b = (*b)[:0]
p.buckets[i].Put(b)

which is IMO not that good :/ and it's like that in multiple places :/

[GiedriusS]

It sounds to me like that is just your personal preference when it brings tangible benefits especially considering how in those code paths a lot of allocations/deallocations are happening. I don't see anything wrong with that code: we just simply want to reuse the slice. Plus, IMHO it is probably not without reason that even Go developers themselves recommend this since using ptrs to slices avoids pointless allocations which is the point of sync.Pool to begin with.

[brancz]

I agree with @GiedriusS, the code paths using sync.Pool are intentionally optimizing allocations, so I don't think this is over-optimizing, it's exactly what should be done in these code paths in my opinion.

[povilasv]

If it's an optimization, then there should be some benchmarks, as right now we are going blind.

AFAIK non pointer stuff is easier on escape analysis and inlining as there are no references to the thing.

https://github.com/golang/go/wiki/CompilerOptimizations#escape-analysis-and-inlining

Gc compiler does global escape analysis across function and package boundaries. However, there are lots of cases where it gives up. For example, anything assigned to any kind of indirection (*p = ...) is considered escaped.

Note that we changed the code to *b = ... in some places

So you could argue the other way around.

So I'm really not sure whether we get less allocations here.

[GiedriusS]

There is a small benchmark - I have pasted the links in the comments and in the original ticket. Do you think it's wrong? Either way, so a slice is a pointer itself with extra information - aren't you contradicting yourself?

[povilasv]

Where am I contradicting myself?

As the go wiki specifically states if you do:

For example, anything assigned to any kind of indirection (*p = ...) is considered escaped.

the variables has escaped, meaning no inlining. We have changed the code to do that, so it would definitely escape.

There is a small benchmark - I have pasted the links

Doesn't consider our code base properly, we would need proper benchmarks like https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go

which shows the difference in allocs.

Right now I'm not saying that we did improve nor saying that we regressed in the memory allocs.

I'm just saying that we don't know, really and we should know.