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

Combine capacity guidance for maps and slices #97

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

mway
Copy link
Contributor

@mway mway commented Jun 13, 2020

Per @prashantv's feedback on #79:

Suggestion: Rather than having 2 separate topics with similar guidelines, what if this was one guideline with sub-headings for maps vs slices.

We can have the common reasons, then have type-specific examples + reasons in the sub-headings.

This also expands a bit more on map allocations at initialization time.

@mway mway requested review from abhinav and prashantv June 13, 2020 15:27
@mway mway force-pushed the mway/condense-capacity-guidance branch 2 times, most recently from 5a87129 to 8011d9b Compare June 13, 2020 15:31
Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just left some small comments

style.md Outdated Show resolved Hide resolved
style.md Outdated
Comment on lines 1725 to 1727
Specify container capacity where possible, as the compiler will allocate all
required memory for that capacity up front rather than potentially allocating
it in chunks as the container grows.
Copy link
Contributor

Choose a reason for hiding this comment

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

The mention of the compiler seems a little too detailed here (since technically, most of the performance guidance affects what the compiler will do, though we don't call it out explicitly).

I also want to emphasize that it's not just that we allocate the memory in chunks (it might seem like instead of allocating 1mb, we'll allocate 5x200kb), but that we'll end up constantly resizing which increases total allocations, and also total time spent due to copies.

Perhaps:
"Specify container capacity where possible to allocate all required memory for the container up front. This avoids copies + resizing of the container as elements are added."

would replace mention of the compiler here (since it can happen at runtime for heap allocations), and the issue isn't just the chunks, but the resizing (e.g., overall more memory allocated + copies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think you're right. I generalized this a bit more to apply more correctly to both slices and maps (since map capacity hints don't guarantee that you'll "allocate all required memory for the container up front" obv) - not trying to be overly pedantic, just want the text to be accurate. Let me know what you think.

style.md Outdated Show resolved Hide resolved
style.md Outdated Show resolved Hide resolved
style.md Outdated Show resolved Hide resolved
@mway mway force-pushed the mway/condense-capacity-guidance branch from 8011d9b to 92d7cac Compare June 16, 2020 19:09
style.md Outdated
### Prefer Specifying Container Capacity

Specify container capacity where possible in order to allocate memory for the
container up front. This minimizes subsequent allocations (by copying and
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: I think there's two spaces before "This"

style.md Outdated
Unlike maps, slice capacity is not a hint: the compiler will allocate enough
memory for the capacity of the slice as provided to `make()`, which means that
subsequent `append()` operations will incur zero allocations (until the length
of the slice matches the capacity, which will require a resize to hold
Copy link
Contributor

Choose a reason for hiding this comment

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

s/which will require a resize/after which any appends will require a resize/

@mway mway merged commit fdb2331 into master Jun 16, 2020
@mway mway deleted the mway/condense-capacity-guidance branch June 16, 2020 19:18
mmelnyk pushed a commit to mmelnyk/guide that referenced this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants