Add prealloc check

[tklauser]

Run https://github.com/alexkohler/prealloc as part of the precheck make target. See #10716 and #10751 for related fixes found by this tool.

Useful for #10056

Reviewable by commit, the first 3 commits are cleanups unrelated to the actual functionality.

[tklauser]

test-me-please

[tklauser]

Need to build a new docker-builder first.

[coveralls]

Coverage increased (+0.02%) to 45.472% when pulling d0c1883 on pr/tklauser/prealloc-check into ad9ccef on master.

[tklauser]

test-me-please

[tklauser]

prealloc needs to be available in the dev VM tool as well (thus the failed Ginkgo-Tests). Created PR cilium/packer-ci-build#204 to add it.

[rolinh]

What's the rationale for requiring to always prealloc slices? I understand that preallocating is important for performance-sensitive code sections but I fear that requiring to always prealloc slices has potential to introduce subtle bugs (by getting the length wrong typically or if the loop body is later modified to not iterate over all elements, etc).
If this is already decided, shouldn't it be mentioned in a coding guideline in the docs?

[tklauser]

What's the rationale for requiring to always prealloc slices? I understand that preallocating is important for performance-sensitive code sections but I fear that requiring to always prealloc slices has potential to introduce subtle bugs (by getting the length wrong typically or if the loop body is later modified to not iterate over all elements, etc).
If this is already decided, shouldn't it be mentioned in a coding guideline in the docs?

It's not requiring to always preallocate slices, only if it detects that the slice is later appended to in a loop. Also, the size of the slice should always be 0 in these cases, we only set the capacity in these cases. Of course that's something that lead to potential bugs, i.e. if people use make([]foo, len(bar) instead of make([]foo, 0, len(bar) but that might also happen in other cases unrelated to preallocations and is something that reviewers hopefully catch.

This isn't decided yet but it was something that people suggested while reviewing #10716 and #10751. I can create a thread on Cilium slack to discuss this further if somebody feels the need for it.

[tklauser]

What's the rationale for requiring to always prealloc slices?

Sorry, forgot to answer this part: the rationale here is to pre-allocate slices where we already know the capacity in order to avoid re-allocations in a loop, which could temporarily spike memory usage. The Go runtime is known to be slow to return freed memory to the OS since Go 1.13, so from the OS perspective it might look like the process allocated more memory than it actually is using. So pre-allocating in this cases helps to reduce memory churn.

[joestringer]

I fear that requiring to always prealloc slices has potential to introduce subtle bugs (by getting the length wrong typically or if the loop body is later modified to not iterate over all elements, etc).

This is a good concern to have, it's a well-known shortcoming of Go slice syntax. Are there tools we can enable by default to help us to make sure we don't mess this up? Or some best practices we document to avoid this? Relying on code review from others to pick up on bugs in this area is not terribly reliable though I think that's partly what we're relying on right now.

[rolinh]

the rationale here is to pre-allocate slices where we already know the capacity in order to avoid re-allocations in a loop, which could temporarily spike memory usage. The Go runtime is known to be slow to return freed memory to the OS since Go 1.13, so from the OS perspective it might look like the process allocated more memory than it actually is using. So pre-allocating in this cases helps to reduce memory churn.

Note that by forcing to preallocate, you might also end up allocating more memory than necessary which works against your goal of reducing overall memory consumption as part of #10056.

For example, take this code from bugtool/cmd/root.go that is modified as part of this PR to preallocate the ciliumPods slice:

func getCiliumPods(namespace, label string) ([]string, error) {
	output, err := execCommand(fmt.Sprintf("kubectl -n %s get pods -l %s", namespace, label))
	if err != nil {
		return nil, err
	}

	lines := strings.Split(output, "\n")
	ciliumPods := make([]string, 0, len(lines))
	for _, l := range lines {
		if !strings.HasPrefix(l, "cilium") {
			continue
		}
		// NAME           READY     STATUS    RESTARTS   AGE
		// cilium-cfmww   0/1       Running   0          3m
		// ^
		pod := strings.Split(l, " ")[0]
		ciliumPods = append(ciliumPods, pod)
	}
	return ciliumPods, nil
}

In a real-case scenario, I'd expect many more non-cilium pods in a cluster than cilium pods. Let's assume that each node hosts 10 pods and among these 10 you have 2 cilium pods (agent+operator). With this example with conservative numbers, you already end up allocating 5 times more memory than necessary as most lines will be skipped in the loop.

[tklauser]

the rationale here is to pre-allocate slices where we already know the capacity in order to avoid re-allocations in a loop, which could temporarily spike memory usage. The Go runtime is known to be slow to return freed memory to the OS since Go 1.13, so from the OS perspective it might look like the process allocated more memory than it actually is using. So pre-allocating in this cases helps to reduce memory churn.

Note that by forcing to preallocate, you might also end up allocating more memory than necessary which works against your goal of reducing overall memory consumption as part of #10056.

For example, take this code from bugtool/cmd/root.go that is modified as part of this PR to preallocate the ciliumPods slice:

func getCiliumPods(namespace, label string) ([]string, error) {
	output, err := execCommand(fmt.Sprintf("kubectl -n %s get pods -l %s", namespace, label))
	if err != nil {
		return nil, err
	}

	lines := strings.Split(output, "\n")
	ciliumPods := make([]string, 0, len(lines))
	for _, l := range lines {
		if !strings.HasPrefix(l, "cilium") {
			continue
		}
		// NAME           READY     STATUS    RESTARTS   AGE
		// cilium-cfmww   0/1       Running   0          3m
		// ^
		pod := strings.Split(l, " ")[0]
		ciliumPods = append(ciliumPods, pod)
	}
	return ciliumPods, nil
}

In a real-case scenario, I'd expect many more non-cilium pods in a cluster than cilium pods. Let's assume that each node hosts 10 pods and among these 10 you have 2 cilium pods (agent+operator). With this example with conservative numbers, you already end up allocating 5 times more memory than necessary as most lines will be skipped in the loop.

Note that in the example above we use k8s-app=cilium by default to only get the cilium pods. So in the usual case (unless the user changes --k8s-label) I'd expect all pods in the list to match.

Your point is valid in general though and we should make sure to only preallocate in cases where we're really sure that we will use as many entries into the slice. For cases where this wasn't clear, I changed slice definitions to slice := make([]foo, 0) to silence prealloc and still not overallocate. Of course it remains to be discussed whether it's worth to do that just to silence a static analyzer, but in the overall changes there were maybe 1-2 out of ~25 cases.

[rolinh]

Note that in the example above we use k8s-app=cilium by default to only get the cilium pods. So in the usual case (unless the user changes --k8s-label) I'd expect all pods in the list to match.

Fair enough in this case :) I was just trying to point out a potential issue with this approach.

Your point is valid in general though and we should make sure to only preallocate in cases where we're really sure that we will use as many entries into the slice.

If I recall correctly, internally, when slices are reallocated their capacity is doubled for every reallocation (can't find a reference atm). In practice, depending on the case, it might be that using make(..., 0, len(foo)) ends up allocating less memory compared to not preallocating even if not as many entries are used. Or it might be more, it all depends on the case :)

I guess that what I'm trying to say is that the question of memory optimization is not so simple after all and having a tool that complains every time a slice could be preallocated might not be the best thing. What I fear more is the introduction of subtle bugs. For instance, a case where a contributor sees the warning, decides to make(..., len(foo)) and as the loop was append'ing, you end up with a bunch of zero-valued elements before the elements that were appended to the slice.

Maybe the solution would be as simple as patching alexkohler/prealloc so that is outputs something like "missing prealloc, consider using make(…, 0, X)" rather than "Consider preallocating X".

[tklauser]

Your point is valid in general though and we should make sure to only preallocate in cases where we're really sure that we will use as many entries into the slice.

If I recall correctly, internally, when slices are reallocated their capacity is doubled for every reallocation (can't find a reference atm). In practice, depending on the case, it might be that using make(..., 0, len(foo)) ends up allocating less memory compared to not preallocating even if not as many entries are used. Or it might be more, it all depends on the case :)

Reallocation seems to depend on the size of the stored type: https://play.golang.org/p/EABWUjgAJww And it might also change depending on Go version.

But yeah, agree that it all depends on the case :)

I guess that what I'm trying to say is that the question of memory optimization is not so simple after all and having a tool that complains every time a slice could be preallocated might not be the best thing. What I fear more is the introduction of subtle bugs. For instance, a case where a contributor sees the warning, decides to make(..., len(foo)) and as the loop was append'ing, you end up with a bunch of zero-valued elements before the elements that were appended to the slice.

Fully agree. As @joestringer already pointed out Go's make syntax for slices and the slice syntax itself is easy to get wrong. IIIRC go vet has or had a check related to slice usage, but I currently can't find it :(

In any case I think we also have the problem of developers getting it wrong without the use of the prealloc tool. Though, I agree it might lead to an increased use of make and thus potentially buggy usage.

Maybe the solution would be as simple as patching alexkohler/prealloc so that is outputs something like "missing prealloc, consider using make(…, 0, X)" rather than "Consider preallocating X".

Thanks for the suggestion, I like that solution. Will prepare a PR against upstream and hope they will take it.

[vadorovsky]

Only one nit. The rest looks good.

You mentioned some false positives reported by prealloc on the weekly meeting. I guess that's not the case anymore? :)

[rolinh]

If I recall correctly, internally, when slices are reallocated their capacity is doubled for every reallocation (can't find a reference atm). In practice, depending on the case, it might be that using make(..., 0, len(foo)) ends up allocating less memory compared to not preallocating even if not as many entries are used. Or it might be more, it all depends on the case :)

Reallocation seems to depend on the size of the stored type: https://play.golang.org/p/EABWUjgAJww And it might also change depending on Go version.

https://github.com/golang/go/blob/801cd7c84d42dcf18256416524aa0d31d6305830/src/runtime/slice.go#L95-L114

	newcap := old.cap
	doublecap := newcap + newcap
	if cap > doublecap {
		newcap = cap
	} else {
		if old.len < 1024 {
			newcap = doublecap
		} else {
			// Check 0 < newcap to detect overflow
			// and prevent an infinite loop.
			for 0 < newcap && newcap < cap {
				newcap += newcap / 4
			}
			// Set newcap to the requested cap when
			// the newcap calculation overflowed.
			if newcap <= 0 {
				newcap = cap
			}
		}
	}

In the end it's a little more subtle that just doubling. Anyway, this was just to satisfy curiosity and as you pointed out, it might change depending on the version of Go.

[tklauser]

If I recall correctly, internally, when slices are reallocated their capacity is doubled for every reallocation (can't find a reference atm). In practice, depending on the case, it might be that using make(..., 0, len(foo)) ends up allocating less memory compared to not preallocating even if not as many entries are used. Or it might be more, it all depends on the case :)

Reallocation seems to depend on the size of the stored type: https://play.golang.org/p/EABWUjgAJww And it might also change depending on Go version.

https://github.com/golang/go/blob/801cd7c84d42dcf18256416524aa0d31d6305830/src/runtime/slice.go#L95-L114

[...]

In the end it's a little more subtle that just doubling. Anyway, this was just to satisfy curiosity and as you pointed out, it might change depending on the version of Go.

Nice, thanks for digging this up. It might even be a bit more involved as it looks like the compiler might be able to optimize some cases before calling runtime.growslice:

https://github.com/golang/go/blob/801cd7c84d42dcf18256416524aa0d31d6305830/src/cmd/compile/internal/gc/ssa.go#L2751-L2783

[aanm]

LGTM but I wonder if we should drop the last commit of #10823

[tklauser]

Rebased after #10823 was merged. Also built a new cilium-builder:2020-04-03 image.

Before this can be successfully tested, cilium/packer-ci-build#204 needs to be merged and a new box image be built.

[tklauser]

As discussed in today's Cilium community meeting, we will not enable prealloc checks by default. Instead, I'll volunteer run it once per cycle and fix any issues where it actually avoids re-allocations and doesn't over-allocate.

I'll close this PR for now and will reopen it the remaining fixes which make sense.

[tklauser]

Sent PR #10913 instead.