Methods for growing and shrinking an array #11485

beta-ziliani · 2021-11-23T21:01:42Z

Solves #8209 and #11443 by creating two methods, one for growing the array and one for shrinking:

ary = ['a', 's', 't', 'e', 'r', 'i', 't', 'e']
p ary.remaining_capacity  # => 8
ary.ensure_capacity(20)
p ary.remaining_capacity  # => 20
ary.trim_to_size(8)
p ary.remaining_capacity  # => 8

spec/std/array_spec.cr

oprypin · 2021-11-23T23:09:16Z

I don't think resize is a good name, because at least in C++, C#, Python+NumPy that means actually dropping elements or filling the array with actual new elements.
C++ has reserve (which, by the way, actually can't release memory) and shrink_to_fit for this purpose.
And perhaps this pull request's title should mention what the method does.

Fryguy · 2021-11-23T23:21:45Z

Perhaps .capacity= (maybe with a getter .capicity)?

straight-shoota · 2021-11-23T23:29:11Z

I agree that rescale might be confusing about the semantics. #capacity= looks better. But it's a setter, not an action method, so that's a bit of a down side as well. It doesn't immediately convey that it might actually move things around.
Maybe #rescale could be an option? Or #resize_capacity?

oprypin · 2021-11-23T23:42:38Z

FWIW doing the API exactly as in C++ would solve all of the issues named so far. It also prevents having to think what happens when you specify a smaller capacity than now. Also I think it's very pragmatic or goal-oriented. I don't often see the need to set a specific smaller buffer size.
So, reserve(size) (never shrinks) and shrink_to_fit()

beta-ziliani · 2021-11-24T08:44:15Z

If we go with two different methods, I like Java's ArrayList ensureCapacity and trimToSize better. Not only the name is different: it tells you that it's ok to give a small value, just it won't do anything.

C# also uses trimToSize, but for the capacity it has a setter as proposed by @Fryguy (that also shrinks). But I don't think the setter is a good idea, since as mentioned in the comment we need to do something about the offset, requiring an extra (defaultable) arg.

straight-shoota · 2021-11-24T18:29:12Z

spec/std/array_spec.cr

+      expect_raises(ArgumentError, "Insufficient capacity: 2 cannot hold 3 elements") do
+        ary = [1, 2, 3]
+        ary.ensure_capacity(2)


Again (#11485 (comment)) this seems very unexpected: Why would this method raise? We ask to ensure the array has at least a capacity of 2, and the actual capacity is 3. So everything should be good, nothing to do.

Agh, sorry I missed this in the original comment. Will fix.

straight-shoota · 2021-11-24T18:36:49Z

src/array.cr

+
+  # Reduces the internal buffer to exactly fit the number of elements in the
+  # array. If `rewind` is truthy it moves the elements in the internal array to the front.
+  def trim_to_size(rewind = true) : self


I'm not convinced about the usefulness of the rewind parameter. #trim_to_size should always rewind. I can't see a relevant usecase where you would not want that, especially since you have no external insight on or control of the offset.
For #ensure_capacity it's not that clear, but I don't know why it should bother about that either.

would it be useful to have an "extra capacity" argument, that is resize_to_capacity(size + extra_capacity)? I think we would want that, so that if an element was added immediately after the trimming, it wouldn't needed to increase capacity so close again. on the other hand i guess ensure_capacity can be used for that.

Yeah, I think the use case for #trim_to_size would be to say I'm done with adding or removing things, let's have it trimmed to save space.

As it is implemented here, ensure_capacity won't shrink, so it won't help you in that case. This is why my original proposal had just one method to set the capacity, either expanding or shrinking it. I wonder what's best, if to go back to the original proposal or to add an extra-capacity argument to leave some room.

I wonder if fixing #11445 would help here in your use case @carlhoerberg .

no, it's a very good addition, but our deques/arrays also shrinks. well, we can continue to do what we do, create a new array/deque and copy all elements to it, but not having to copy would be much more efficent.

I would say that we indeed should never expose the rewind parameter and always do it.
Later someone can separately research whether that's a significant slowdown, and then possibly add this parameter. Also whether it's perhaps possible to do a 2-in-1 equivalent of realloc + rewind.

But, then again, potentially deprecating the rewind parameter to be ignored (always true) would also not be a breaking change. So maybe it's fine as is.

Please let's not add the rewind parameter. Nothing in Array's API says anything about the internal implementation. This will prevent changing the internal implementation later on. Also users reading this will have no idea what this means.

Fryguy · 2021-11-24T19:19:12Z

src/array.cr

+      resize_to_capacity capacity
+    else
+      err = "Insufficient capacity: #{capacity} cannot hold #{@size} elements"
+      err += " with an offset of #{@offset_to_buffer}" if @offset_to_buffer > 0


Similar to the discussion in https://github.com/crystal-lang/crystal/pull/11485/files#r756340774, this offset part of the error message is an internal implementation detail that users will likely not understand.

asterite · 2021-11-25T10:11:02Z

Where did the requirement for introducing these methods come from? I don't doubt their usefulness, I just wonder if there are real programs where you tried this and it proved to be a benefit.

I'm only asking this because Ruby doesn't have such methods.

carlhoerberg · 2021-11-25T13:55:33Z

We have Deques/Arrays that can grow to >100M elements, and then back to 0 or any value in between, and for our long running server application we want to minimize memory usage, hence what we do today when we notice that the size of the Deque/Array is significatly smaller than the capacity is to create a new Deque/Array and copies all the remaining elements to it, so that the unused memory can be GCed, and returned to the OS.

asterite · 2021-11-25T15:46:41Z

Wow! 100M elements in memory? Without knowing anything about what the application is... Can't that data be kept in a database? Something like sqlite3, for example.

straight-shoota · 2021-11-25T16:46:39Z

@carlhoerberg So you occasionally want to trim down the capacity, but not as tight as the current size, in order to leave some room for growing again without resizing. Sounds legit.
Note however, that trimming does realloc, which may do a copy as well. So it's is not necessarily significantly faster than allocating a new array. It has an opportunity to be faster, depending on what realloc does.

beta-ziliani · 2021-11-29T14:24:38Z

I removed the rewind parameters (it always rewind first), and I added an extra capacity to trim_to_size to have an option of leaving some spare room.

oprypin · 2021-11-30T18:11:08Z

src/array.cr

+    raise ArgumentError.new("Negative extra capacity: #{extra}") if extra < 0
+
+    rewind
+    resize_to_capacity (@size + extra)


Hmm there's an extra space here; surprised crystal tool format didn't complain

Interesting. I suppose that's because it's just interpreted as method call plus a separate parenthesized expression, not as a method call with args in parenthesis.

oprypin · 2021-11-30T18:11:17Z

src/array.cr

+    shift_buffer_by -@offset_to_buffer
+  end
+
+  # Enusures that the internal buffer has at least `capacity` elements.


Typo "ensures"

Sija · 2021-11-30T18:46:00Z

src/array.cr

+
+  # Enusures that the internal buffer has at least `capacity` elements.
+  def ensure_capacity(capacity : Int32) : self
+    rewind


This will do a realloc even if the given capacity is smaller than @size, is that intended?

It doesn't realloc, but memcpy. But that's the same problem.
I'm not sure #ensure_capacity should ever rewind.

There's never a need to rewind.

asterite · 2021-11-30T22:30:46Z

src/array.cr

+
+  # Reduces the internal buffer to exactly fit the number of elements in the
+  # array, plus `extra` elements.
+  def trim_to_size(extra : Int32 = 0) : self


This extra argument is pretty confusing. Trim to size 2 looks like it will trim it to size 2. I would rename extra to "plus", and make it a named argument. Then it will read like "trim to size plus 2".

Hmm I'm not sure I agree on the word. extra is fully correct English, I slightly prefer that one. "with 2 extra elements".
But yes, definitely would need to remove the ability to pass this as a positional argument 👍

Yes, it's mainly that as a positional argument a reader might be confused. The named argument could still be extra, but then it doesn't read like an English sentence. "trim to size extra 2" vs "trim to size plus 2". Maybe there's another word or set of words we could use.

Vote:

❤️ a.trim_to_size(plus: 2)
😄 a.trim_to_size(extra: 2)
🎉 a.trim_to_size(with_extra: 2)

I can't think of another relevant name.

I dislike the idea of naming a parameter with_anything. My impression was that the "with" is implied by the opening of the list of arguments -- of course you're calling the method with something. I was going to make an argument that by that logic you could add with_ to any parameter whatsoever, but actually I didn't find any examples, so it could just be that I'm wrong here.

Maybe bonus? :)

Or more technical additive.

HertzDevil · 2021-12-01T14:49:50Z

Any reason that extra space can be reserved only on the right (for #push) but not on the left (for #unshift)?

beta-ziliani · 2021-12-01T14:55:24Z

@HertzDevil yes: delay a proper treatment of @offset_to_buffer.

method

b613bb9

straight-shoota reviewed Nov 23, 2021

View reviewed changes

spec/std/array_spec.cr Outdated Show resolved Hide resolved

jkthorne reviewed Nov 23, 2021

View reviewed changes

spec/std/array_spec.cr Outdated Show resolved Hide resolved

splitting into two methods, ensure_capacity and trim_to_size

ede4526

beta-ziliani changed the title ~~New Array#resize method~~ Methods for growing and shrinking an array Nov 24, 2021

straight-shoota reviewed Nov 24, 2021

View reviewed changes

Fryguy reviewed Nov 24, 2021

View reviewed changes

HertzDevil added kind:feature topic:stdlib:collection labels Nov 25, 2021

it's ok to ask for fewer capacity

8b5aba1

oprypin self-requested a review November 26, 2021 20:59

always rewind, and extra capacity for trimming

97232a1

beta-ziliani mentioned this pull request Nov 29, 2021

Expose capacity in an array #11507

Open

oprypin reviewed Nov 30, 2021

View reviewed changes

Sija reviewed Nov 30, 2021

View reviewed changes

asterite reviewed Nov 30, 2021

View reviewed changes

beta-ziliani added 3 commits December 1, 2021 10:12

Merge branch 'master' into array_resize

4a6e885

merged master

199e71e

no rewinding in ensure_capacity

796a82c

democracy spoken: extra it is

96ff0bf

straight-shoota mentioned this pull request Dec 1, 2021

[Formatter] Fix space between call name and parenthesized arg #11523

Merged

beta-ziliani mentioned this pull request Dec 29, 2021

Array reallocation API #8209

Open

snacks02 mentioned this pull request Feb 28, 2023

Make Array#@buffer a part of the public API #13124

Closed

beta-ziliani added 2 commits November 6, 2024 14:31

Merging master

c5850bc

grammar fixes

9366bd2

Methods for growing and shrinking an array #11485

Are you sure you want to change the base?

Methods for growing and shrinking an array #11485

Conversation

beta-ziliani commented Nov 23, 2021 • edited Loading

oprypin commented Nov 23, 2021

Fryguy commented Nov 23, 2021

straight-shoota commented Nov 23, 2021

oprypin commented Nov 23, 2021 • edited Loading

beta-ziliani commented Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Nov 25, 2021

carlhoerberg commented Nov 25, 2021 • edited Loading

asterite commented Nov 25, 2021

straight-shoota commented Nov 25, 2021

beta-ziliani commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beta-ziliani Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HertzDevil commented Dec 1, 2021

beta-ziliani commented Dec 1, 2021

beta-ziliani commented Nov 23, 2021 •

edited

Loading

oprypin commented Nov 23, 2021 •

edited

Loading

beta-ziliani commented Nov 24, 2021 •

edited

Loading

carlhoerberg commented Nov 25, 2021 •

edited

Loading

beta-ziliani Dec 1, 2021 •

edited

Loading