Skip to content

Replace loop in ensureCapacity with a std.mem.max for the new capacity. #1233

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Replace loop in ensureCapacity with a std.mem.max for the new capacity. #1233

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions std/array_list.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,8 @@ pub fn AlignedArrayList(comptime T: type, comptime A: u29) type {
pub fn ensureCapacity(self: *Self, new_capacity: usize) !void {
var better_capacity = self.items.len;
if (better_capacity >= new_capacity) return;
while (true) {
better_capacity += better_capacity / 2 + 8;
if (better_capacity >= new_capacity) break;
}
better_capacity += better_capacity / 2 + 8;
better_capacity = std.mem.max(usize, []usize{better_capacity, new_capacity});
Copy link
Contributor

@isaachier isaachier Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't what you want. You need to keep multiplying better_capacity by two and adding 8 until you are greater than new_capacity. The loop actually accomplishes this mathematical recurrence: f(n) = f(n - 1) + f(n - 1) / 2 + 8. The new code does not yield the same result.

Edit: thanks @tiehuis for correcting recurrence.

Copy link
Member

Choose a reason for hiding this comment

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

Just clarifying, but the current factor is 1.5x f(n) = f(n-1) + f(n-1) / 2 + 8 which is arguably a better ratio than 2. We want to do this in a loop because it preserves the property that a resize will always slightly overallocate on the assumption that more items beyond the point will eventually be added.

It could be worthwhile in some cases to have an ensureCapacityExact which resizes to the exact amount if it is less than.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know the new code doesn't do the same as what the old code does. But I don't think what the old code does makes any sense. I can see that you might want slightly more than asked for, but there are simpler ways to achieve that than having a loop.

ps. Why is 1.5 better than 2? If you just append elements in a tight loop 2 is better than 1.5 (and probably >2 is even better) because you just do less allocations and copies.

pps. Just for reference. For std::vector libstdc++ has 2 and the microsoft one has 1.5. And both just take the greater of the equivalent of better_capacity and new_capacity.

Copy link

Choose a reason for hiding this comment

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

https://github.com/facebook/folly/blob/master/folly/docs/FBVector.md

Reasoning for the 2 vs 1.5 discussion

Copy link
Contributor Author

@BarabasGitHub BarabasGitHub Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense. I wonder how much that affects real life programs. However, they don't always use 1.5.
https://github.com/facebook/folly/blob/f6f9d7ead19c34c033c4b673cc7ecb2942a6cb4a/folly/FBVector.h#L1186

By the way the +8 also pushes back the point of reusing memory significantly when starting with small sizes.

Reading that piece also makes me wonder about the allocators (and realloc). Maybe they could return a larger size than you requested if that suits them better.

self.items = try self.allocator.alignedRealloc(T, A, self.items, better_capacity);
}

Expand Down