Skip to content

proposal: slices: Use make instead of Grow in Concat #65884

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

Closed
justinhwang opened this issue Feb 22, 2024 · 6 comments
Closed

proposal: slices: Use make instead of Grow in Concat #65884

justinhwang opened this issue Feb 22, 2024 · 6 comments
Labels
Milestone

Comments

@justinhwang
Copy link

Proposal Details

slices.Concat unnecessarily uses Grow instead of make which unnecessarily increases time/op

@gopherbot gopherbot added this to the Proposal milestone Feb 22, 2024
@justinhwang
Copy link
Author

@justinhwang justinhwang closed this as not planned Won't fix, can't repro, duplicate, stale Feb 22, 2024
@OhJuhun
Copy link

OhJuhun commented Jul 15, 2024

@justinhwang
Hello. Why this issue is closed?
I think builtin make is better than slices.Grow.
Sure, in some situations slices.Grow may be more appropriate, but in this case, builtint make seems more suitable.
But in this case, slices.Grow is not better than make in terms of performance, readability, or memory allocation.
I read your discussion on PR, but I can't understand about that.
Because I think the background posting is wrong:
This code DO NOT allocate memories.

func BenchmarkSortStrings(b *testing.B) {
        s := []string{"heart", "lungs", "brain", "kidneys", "pancreas"}
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
                sort.Strings(s)
        }
}

Therefore, I believe that this post is no longer valid.

Do you understand why slices.Grow replace builtin make?
If you do, can you explain for me please?

@ianlancetaylor
Copy link
Contributor

See the discussion on https://go.dev/cl/504882.

@OhJuhun
Copy link

OhJuhun commented Jul 16, 2024

@ianlancetaylor Thank you for your reply.
But, I already read this discussion.
"But I feel like I didn't fully understand the comment's exact meaning. I agree that there is no need to explicitly set the capacity; there is no particular reason for doing so. However, the variable name generated internally is also 'newSlices,' and the 'Grow' used in Concat always returns a slice with the size of 'size.' I believe the intent of 'Grow' is to increase the size of the provided slice, not to create a new slice. But in Concat, a new slice is being created by passing 'nil' as the original slice to 'Grow.' Considering this, I am curious if there is really a need to use 'Grow' instead of 'make' from a readability perspective. Performance also seems better when generating 'newSlices' using 'make.' I don't quite understand what issues can arise from guaranteeing the capacity. Could you please explain this in more detail? Sorry to trouble you."

image

@randall77
Copy link
Contributor

The reason we want to round up to the next size class is to avoid reallocation on a subsequent append.

s := slices.Concat(...)
s = append(s, 7)

With make, that second line will always do a reallocation+copy. If we round up to a size class using Grow, then probably that second line will not have to do that extra work.

@OhJuhun
Copy link

OhJuhun commented Jul 16, 2024

@randall77
I misunderstood about slices.Grow.
I missed the comment: After Grow(n), "AT LEAST" n elements can be appended
Thank you for your reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants