Run plan.Clean() upon error too + refactor pointslicepool

[Dieterbe]

No description provided.

[shanson7]

Generally looks good, couple comments.

FYI, we recently disabled plan.Clean() due to some (very rare) corruption in responses we were seeing. Still waiting to see if disabling plan.Clean() fixes it. If it does, we'll need to do another round to figure out what is going on.

[shanson7]

A lot of functions know how many datapoints they need and could use GetMin

[Dieterbe]

true, but i don't have time now to go over all those

[shanson7]

I'm wondering if we want to allocate a default slice here or just let it return nil. If someone calls GetMin(2001) and we didn't have anything in the pool, we will allocate two slices (capacities 2000 and 2001).

[Dieterbe]

If we return nil, then we put quite some burden on callers that call Get rather than GetMin. If those callers want to be efficient, they have to mind their own allocations, which defeats the point.

I suggest that if or when we do the work to transition call sites to GetMin, we can then apply your suggestion. Or even better, remove the Get method alltogether and make the hint mandatory.

[shanson7]

I think you misunderstood. Basically, no callers should ever access PointSlicePool.p directly. This means PointSlicePool.Get() is free to just be an alias for GetMin(100) (or whatever) and GetMin has the logic to allocate if nil is returned.

[Dieterbe]

Basically, no callers should ever access PointSlicePool.p directly

sure. to be clear, this is already the case. (.p is a private property so nothing in a different package can access it)

This means PointSlicePool.Get() is free to just be an alias for GetMin(100) (or whatever) and GetMin has the logic to allocate if nil is returned.

Seems like the choice of the number 100 here is the same as the choice of DefaultPointSliceSize, so we may as well use DefaultPointSliceSize here.
so doing what we have ....

                        New: func() interface{} {
                                return make([]schema.Point, 0, defaultSize)
                        },

seems equivalent to calling GetMin(DefaultPointSliceSize), GetMin receiving a nil and then allocating a slice of the requested size.
(sorry i guess i'm still not getting what you're saying)

[shanson7]

seems equivalent to calling GetMin(DefaultPointSliceSize), GetMin receiving a nil and then allocating a slice of the requested size.

For the raw Get case, yes it is the same. But for GetMin(N) where N > DefaultPointSliceSize we save an allocation of DefaultPointSliceSize. For example, GetMin(2001) with an empty pool will currently allocate a cap(2000) slice, put it right back into the pool because it's too small and then allocate cap(2001) slice.

With the suggested implementation GetMin(2001) will get nil from the pool and will allocate the cap(2001) slice. No extra cap(2000) slice allocated.

So, under the assumption that at some point we can update the Get calls (almost) everywhere to use GetMin (which honestly should be very easy because in most funcs it's just len(serie.Datapoints)) then we should see the benefit of this approach.

[Dieterbe]

ok, now i see what you're saying.
how does 3754a85 look ?

one side-effect of this is that in theory regular calls to Get(), which previously would get a small (big enough to be useful, but smaller than p.defaultSize) slice, now would return that slice, and allocate a bigger one instead.
But in practice i don't see that happening because slices being added to the pool should all have come from either Get() or GetMin() anyway

[Dieterbe]

rebased on latest master.
can i get an approve?

[Dieterbe]

@shanson7 :

FYI, we recently disabled plan.Clean() due to some (very rare) corruption in responses we were seeing. Still waiting to see if disabling plan.Clean() fixes it. If it does, we'll need to do another round to figure out what is going on.

is there an update on this? this is probably resolved as of the recent related fixes wrt the double put etc I think?

[shanson7]

@shanson7 :

FYI, we recently disabled plan.Clean() due to some (very rare) corruption in responses we were seeing. Still waiting to see if disabling plan.Clean() fixes it. If it does, we'll need to do another round to figure out what is going on.

is there an update on this? this is probably resolved as of the recent related fixes wrt the double put etc I think?

We haven't actually gotten a chance to rollout the latest fix. The last update I have is that we do conditional plan.Clean() (checking for overlaps as we return the slices to the pool), which is what aided me in finding the bug earlier. I'd like to rollout the fix and keep the "checked" plan.Clean() and ideally see no reoccurrences of the issue.