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

all: preallocate slices with known size #10716

Merged
merged 1 commit into from Mar 28, 2020
Merged

Conversation

tklauser
Copy link
Member

This avoids reallocations in loops, cf. #10056

Found using github.com/alexkohler/prealloc

@tklauser tklauser added pending-review release-note/misc This PR makes changes that have no direct user impact. labels Mar 26, 2020
@tklauser tklauser requested a review from a team March 26, 2020 12:57
@tklauser tklauser requested review from a team as code owners March 26, 2020 12:57
@tklauser tklauser requested review from a team March 26, 2020 12:57
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 26, 2020
@tklauser
Copy link
Member Author

test-me-please

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add this as a checker?

@tklauser
Copy link
Member Author

Can we add this as a checker?

Will look into it, yes. But there are some cases in which preallocation doesn't make sense, i.e. the slice is rather small and/or the capacity cannot be estimated reliably. Thus the prealloc results need manual inspection, e.g. I left out these:

pkg/datapath/linux/node_addressing.go:39 Consider preallocating addresses
pkg/endpoint/bpf.go:258 Consider preallocating updatedStats
pkg/envoy/server.go:1031 Consider preallocating policies

Not sure how to tell the checker to ignore these cases.

@aanm
Copy link
Member

aanm commented Mar 26, 2020

Can we add this as a checker?

Will look into it, yes. But there are some cases in which preallocation doesn't make sense, i.e. the slice is rather small and/or the capacity cannot be estimated reliably. Thus the prealloc results need manual inspection, e.g. I left out these:

pkg/datapath/linux/node_addressing.go:39 Consider preallocating addresses
pkg/endpoint/bpf.go:258 Consider preallocating updatedStats
pkg/envoy/server.go:1031 Consider preallocating policies

Not sure how to tell the checker to ignore these cases.

I guess there isn't a tag to ignore false positives.

@coveralls
Copy link

coveralls commented Mar 26, 2020

Coverage Status

Coverage increased (+0.04%) to 45.546% when pulling 04549f6 on pr/tklauser/prealloc-slices into 5ff7c51 on master.

@tklauser
Copy link
Member Author

I guess there isn't a tag to ignore false positives.

Doesn't look like it 😞

@tklauser
Copy link
Member Author

Found some other cases using the -forloops flag, please have another look.

@tklauser tklauser force-pushed the pr/tklauser/prealloc-slices branch from d01e76c to b25cd23 Compare March 26, 2020 13:48
@tklauser tklauser requested a review from a team as a code owner March 26, 2020 13:48
@tklauser tklauser requested review from tgraf and aanm March 26, 2020 13:48
@tklauser
Copy link
Member Author

test-me-please

This avoids reallocations in loops, cf. #10056

Found using github.com/alexkohler/prealloc

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser force-pushed the pr/tklauser/prealloc-slices branch from b25cd23 to 04549f6 Compare March 26, 2020 23:16
@tklauser
Copy link
Member Author

test-me-please

@tgraf tgraf merged commit 3a49c6c into master Mar 28, 2020
1.8.0 automation moved this from In progress to Merged Mar 28, 2020
@tgraf tgraf deleted the pr/tklauser/prealloc-slices branch March 28, 2020 12:34
tklauser added a commit that referenced this pull request Mar 30, 2020
These were missed in #10716

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
errordeveloper pushed a commit that referenced this pull request Mar 31, 2020
These were missed in #10716

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser mentioned this pull request Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants