From d13f01da977d79e820aeb600ce19cf6ce15fb2c2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 15 Dec 2020 14:44:12 -0300 Subject: [PATCH 1/3] Array: optimize shift --- spec/std/array_spec.cr | 133 +++++++++++++++++++ src/array.cr | 291 +++++++++++++++++++++++++++++++++++------ 2 files changed, 386 insertions(+), 38 deletions(-) diff --git a/spec/std/array_spec.cr b/spec/std/array_spec.cr index cf6fa47cf47f..ccb979394765 100644 --- a/spec/std/array_spec.cr +++ b/spec/std/array_spec.cr @@ -389,6 +389,15 @@ describe "Array" do a.should eq([6, 7, 4, 5]) end + it "optimizes when index is 0" do + a = [1, 2, 3, 4, 5, 6, 7, 8] + buffer = a.@buffer + a[0..2] = 10 + a.should eq([10, 4, 5, 6, 7, 8]) + a.@offset_to_buffer.should eq(2) + a.@buffer.should eq(buffer + 2) + end + it "replaces entire range with a value for empty array (#8341)" do a = [] of Int32 a[..] = 6 @@ -535,6 +544,15 @@ describe "Array" do a.should eq([1, 3, 4]) end + it "deletes at beginning is same as shift" do + a = [1, 2, 3, 4] + buffer = a.@buffer + a.delete_at(0) + a.should eq([2, 3, 4]) + a.@offset_to_buffer.should eq(1) + a.@buffer.should eq(buffer + 1) + end + it "deletes use range" do a = [1, 2, 3] a.delete_at(1).should eq(2) @@ -1173,6 +1191,101 @@ describe "Array" do a.shift(-1) end end + + it "shifts one and resizes" do + a = [1, 2, 3, 4] + old_capacity = a.@capacity + a.shift.should eq(1) + a.@offset_to_buffer.should eq(1) + a << 5 + a.@capacity.should eq(old_capacity * 2) + a.@offset_to_buffer.should eq(1) + a.size.should eq(4) + a.should eq([2, 3, 4, 5]) + end + + it "shifts almost all and then avoid resize" do + a = [1, 2, 3, 4] + old_capacity = a.@capacity + (1..3).each do |i| + a.shift.should eq(i) + end + a.@offset_to_buffer.should eq(3) + a << 5 + a.@capacity.should eq(old_capacity) + a.@offset_to_buffer.should eq(0) + a.size.should eq(2) + a.should eq([4, 5]) + end + + it "shifts and then concats Array" do + size = 10_000 + a = (1..size).to_a + (size - 1).times do + a.shift + end + a.size.should eq(1) + a.concat((1..size).to_a) + a.size.should eq(size + 1) + a.should eq([size] + (1..size).to_a) + end + + it "shifts and then concats Enumerable" do + size = 10_000 + a = (1..size).to_a + (size - 1).times do + a.shift + end + a.size.should eq(1) + a.concat((1..size)) + a.size.should eq(size + 1) + a.should eq([size] + (1..size).to_a) + end + + it "shifts all" do + a = [1, 2, 3, 4] + buffer = a.@buffer + 4.times do + a.shift + end + a.size.should eq(0) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end + + it "shifts all after pop" do + a = [1, 2, 3, 4] + buffer = a.@buffer + a.pop + 3.times do + a.shift + end + a.size.should eq(0) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end + + it "pops after shift" do + a = [1, 2, 3, 4] + buffer = a.@buffer + 3.times do + a.shift + end + a.pop + a.size.should eq(0) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end + + it "shifts all with shift(n)" do + a = [1, 2, 3, 4] + buffer = a.@buffer + a.shift + a.shift(3) + a.size.should eq(0) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end end describe "shuffle" do @@ -1452,6 +1565,26 @@ describe "Array" do a.unshift(1, 2, 3).should be(a) a.should eq([1, 2, 3]) end + + it "unshifts after shift" do + a = [1, 2, 3, 4] + buffer = a.@buffer + a.shift + a.unshift(10) + a.should eq([10, 2, 3, 4]) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end + + it "unshifts many after many shifts" do + a = [1, 2, 3, 4, 5, 6, 7, 8] + buffer = a.@buffer + 3.times { a.shift } + a.unshift(10, 20, 30) + a.should eq([10, 20, 30, 4, 5, 6, 7, 8]) + a.@offset_to_buffer.should eq(0) + a.@buffer.should eq(buffer) + end end it "does update" do diff --git a/src/array.cr b/src/array.cr index 7f4cbbc30507..d0478817ae94 100644 --- a/src/array.cr +++ b/src/array.cr @@ -50,14 +50,38 @@ class Array(T) # Size of an Array that we consider small to do linear scans or other optimizations. private SMALL_ARRAY_SIZE = 16 - # Returns the number of elements in the array. - # - # ``` - # [:foo, :bar].size # => 2 - # ``` - getter size : Int32 + # The size of this array. + @size : Int32 + + # The capacity of `@buffer`. + # Note that, because `@buffer` moves on shift, the actual + # capacity (the allocated memory) starts at `@buffer - @offset_to_buffer`. + # The actualy capacity is also given by the `remaining_capacity` internal method. @capacity : Int32 + # Offset to the buffer that was originally allocated, and which needs to + # be reallocated on resize. On shift this value gets increased, together with + # `@buffer`. To reach the root buffer you have to do `@buffer - @offset_to_buffer`, + # and this is also provided by the `root_buffer` internal method. + @offset_to_buffer : Int32 = 0 + + # The buffer where elements start. + @buffer : Pointer(T) + + # In 64 bits the Array is composed then by: + # - type_id : Int32 # 4 bytes -| + # - size : Int32 # 4 bytes |- packed as 8 bytes + # + # - capacity : Int32 # 4 bytes -| + # - offset_to_buffer : Int32 # 4 bytes |- packed as 8 bytes + # + # - buffer : Pointer # 8 bytes |- another 8 bytes + # + # So in total 24 bytes. Without offset_to_buffer it's the same, + # because of aligning to 8 bytes (at least in 64 bits), and that's + # why we chose to include this value, because with it we can optimize + # `shift` to let Array be used as a queue/deque. + # Creates a new empty `Array`. def initialize @size = 0 @@ -153,6 +177,13 @@ class Array(T) ary end + # Returns the number of elements in the array. + # + # ``` + # [:foo, :bar].size # => 2 + # ``` + getter size : Int32 + # Equality. Returns `true` if each element in `self` is equal to each # corresponding element in *other*. # @@ -415,9 +446,32 @@ class Array(T) @buffer[index] = value else diff = count - 1 - (@buffer + index + 1).move_from(@buffer + index + count, size - index - count) - (@buffer + @size - diff).clear(diff) - @buffer[index] = value + + # If index is 0 we can avoid a memcpy by doing a shift. + # For example if we have: + # + # a = ['a', 'b', 'c', 'd'] + # + # and someone does: + # + # a[0..2] = 'x' + # + # we can change the value at 2 to 'x' and repoint `@offset_to_buffer`: + # + # [-, -, 'x', 'd'] + # ^ + # + # (we also have to clear the elements before that) + if index == 0 + @buffer.clear(diff) + shift_buffer_by(diff) + @buffer.value = value + else + (@buffer + index + 1).move_from(@buffer + index + count, size - index - count) + (@buffer + @size - diff).clear(diff) + @buffer[index] = value + end + @size -= diff end @@ -665,26 +719,25 @@ class Array(T) # ``` def concat(other : Array) other_size = other.size - new_size = size + other_size - if new_size > @capacity - resize_to_capacity(Math.pw2ceil(new_size)) - end + + resize_if_cant_insert(other_size) (@buffer + @size).copy_from(other.to_unsafe, other_size) - @size = new_size + + @size += other_size self end # :ditto: def concat(other : Enumerable) - left_before_resize = @capacity - @size + left_before_resize = remaining_capacity - @size len = @size buf = @buffer + len other.each do |elem| if left_before_resize == 0 double_capacity - left_before_resize = @capacity - len + left_before_resize = remaining_capacity - len buf = @buffer + len end buf.value = elem @@ -727,6 +780,11 @@ class Array(T) def delete_at(index : Int) index = check_index_out_of_bounds index + # Deleting the first element is the same as a shift + if index == 0 + return shift_when_not_empty + end + elem = @buffer[index] (@buffer + index).move_from(@buffer + index + 1, size - index - 1) @size -= 1 @@ -791,7 +849,7 @@ class Array(T) # ary2 # => [[5, 2], [3, 4], [7, 8]] # ``` def dup - Array(T).build(@capacity) do |buffer| + Array(T).build(@size) do |buffer| buffer.copy_from(@buffer, size) size end @@ -985,8 +1043,6 @@ class Array(T) # a.insert(-1, "z") # => ["x", "a", "y", "b", "c", "z"] # ``` def insert(index : Int, object : T) - check_needs_resize - if index < 0 index += size + 1 end @@ -995,9 +1051,18 @@ class Array(T) raise IndexError.new end - (@buffer + index + 1).move_from(@buffer + index, size - index) - @buffer[index] = object + # See if we can move backwards if we have space + if index == 0 && @offset_to_buffer > 0 + shift_buffer_by(-1) + @buffer.value = object + else + check_needs_resize + (@buffer + index + 1).move_from(@buffer + index, size - index) + @buffer[index] = object + end + @size += 1 + self end @@ -1497,6 +1562,13 @@ class Array(T) @size -= 1 value = @buffer[@size] (@buffer + @size).clear + + # If we remain empty we also take the chance to + # reset the buffer to its original position. + if empty? && @offset_to_buffer != 0 + reset_buffer_to_root_buffer + end + value end end @@ -1577,11 +1649,13 @@ class Array(T) # ``` def push(*values : T) new_size = @size + values.size - resize_to_capacity(Math.pw2ceil(new_size)) if new_size > @capacity + + resize_if_cant_insert(values.size) + values.each_with_index do |value, i| @buffer[@size + i] = value end - @size = new_size + @size += values.size self end @@ -1728,16 +1802,38 @@ class Array(T) shift { raise IndexError.new } end + # Removes the first value of `self`, at index 0, or otherwise invokes the given block. + # This method returns the removed value. + # If the array is empty, it invokes the given block and returns its value. + # + # ``` + # a = ["a"] + # a.shift { "empty!" } # => "a" + # a # => [] + # a.shift { "empty!" } # => "empty!" + # a # => [] + # ``` def shift if @size == 0 yield else - value = @buffer[0] - @size -= 1 - @buffer.move_from(@buffer + 1, @size) - (@buffer + @size).clear - value + shift_when_not_empty + end + end + + # Internal implementation of shift when we are sure the array is not empty + private def shift_when_not_empty + value = @buffer[0] + @size -= 1 + @buffer.clear(1) + + if empty? + reset_buffer_to_root_buffer + else + shift_buffer_by(1) end + + value end # Removes the first *n* values of `self`, starting at index 0. @@ -1763,9 +1859,15 @@ class Array(T) n = Math.min(n, @size) ary = Array(T).new(n) { |i| @buffer[i] } - @buffer.move_from(@buffer + n, @size - n) @size -= n - (@buffer + @size).clear(n) + + @buffer.clear(n) + + if empty? + reset_buffer_to_root_buffer + else + shift_buffer_by(n) + end ary end @@ -2080,7 +2182,7 @@ class Array(T) self end - # Prepend. Adds *obj* to the beginning of `self`, given that the type of the value is *T* + # Prepend. Adds *object* to the beginning of `self`, given that the type of the value is *T* # (which might be a single type or a union of types). # This method returns `self`, so several calls can be chained. # See `shift` for the opposite effect. @@ -2094,22 +2196,42 @@ class Array(T) # a.unshift("c") # => ["c", "a", "b"] # a.unshift(1) # => [1, "c", "a", "b"] # ``` - def unshift(obj : T) - insert 0, obj + def unshift(object : T) + if @offset_to_buffer > 0 + shift_buffer_by(-1) + @buffer.value = object + else + check_needs_resize + (@buffer + 1).move_from(@buffer, size) + @buffer[0] = object + end + + @size += 1 + self end # Prepend multiple values. The same as `unshift`, but takes an arbitrary number # of values to add to the array. Returns `self`. def unshift(*values : T) - new_size = @size + values.size - resize_to_capacity(Math.pw2ceil(new_size)) if new_size > @capacity + # Check if we have enough offset from the buffer to fit the values + if @offset_to_buffer >= values.size + shift_buffer_by(-values.size) + values.each_with_index do |value, i| + @buffer[i] = value + end + @size += values.size + return self + end + + resize_if_cant_insert(values.size) + move_value = values.size @buffer.move_to(@buffer + move_value, @size) values.each_with_index do |value, i| @buffer[i] = value end - @size = new_size + @size += values.size self end @@ -2119,7 +2241,71 @@ class Array(T) end private def check_needs_resize - double_capacity if @size == @capacity + # We have to compare against the actual capacity in case `@buffer` was moved + return unless @size == remaining_capacity + + # If the array is not empty and more than half of the elements were shifted + # then we avoid a resize and just move the elements to the left. + # This is an heuristic. We could always try to move the elements if + # `@offset_to_buffer` is positive but it might happen that a user does + # `shift` + `push` in succession and it will produce a lot of memcopies. + # + # Note: `@offset_to_buffer != 0` is not redundant because `@capacity` might be 1. + # and so `@capacity / 2` is 0 and `@offset_to_buffer >= @capacity / 2` would hold + # without it. + if @capacity != 0 && @offset_to_buffer != 0 && @offset_to_buffer >= @capacity / 2 + # Given + # + # [-, -, -, 'c', 'd', -] + # | | + # | ^-- `@buffer` + # | + # ^-- root_buffer + # + # and: + # - @size is 2 + # - @capacity is 6 + # - @offset_to_buffer is 3 + # - remaining_capacity is 3 + + # First copy the remaining elements in the array to the front + # + # [-, -, -, 'c', 'd', -] + # ^-------^ + # | + # ^-- copy this + # + # [-, -, -, 'c', 'd', -] + # ^----^ + # | + # ^-- here + # + # We get: + # + # ['c', 'd', '-', 'c', 'd', -] + root_buffer.copy_from(@buffer, @size) + + # Then after that we have to clear the rest of the elements + # + # ['c', 'd', '-', 'c', 'd', -] + # ^-------------^ + # | + # ^-- clear this + # We get: + # + # ['c', 'd', -, -, -, -] + (root_buffer + @size).clear(@offset_to_buffer) + + # Move the buffer pointer to where it was originally allocated, + # and now we don't have any offset to the root buffer + reset_buffer_to_root_buffer + else + double_capacity + end + end + + def remaining_capacity + @capacity - @offset_to_buffer end private def double_capacity @@ -2129,12 +2315,41 @@ class Array(T) private def resize_to_capacity(capacity) @capacity = capacity if @buffer - @buffer = @buffer.realloc(@capacity) + @buffer = root_buffer.realloc(@capacity) + @offset_to_buffer else @buffer = Pointer(T).malloc(@capacity) end end + private def resize_if_cant_insert(insert_size) + # Resize if we exceed the remaining capacity. + # `remaining_capacity - @size` is the actual number of slots we have + # to push new elements. + if insert_size > remaining_capacity - @size + # The new capacity that we need is what we already have occupied + # because of shift (`@offset_to_buffer`) plus my size plus the insert size. + resize_to_capacity(Math.pw2ceil(@offset_to_buffer + @size + insert_size)) + end + end + + # Returns a pointer to the buffer that was originally allocated/reallocated + # for this array. + private def root_buffer + @buffer - @offset_to_buffer + end + + # Moves `@buffer` by n while at the same time increments `@offset_to_buffer` + private def shift_buffer_by(n) + @offset_to_buffer += n + @buffer += n + end + + # Makes `@buffer` point at the original buffer that was allocated/reallocated. + private def reset_buffer_to_root_buffer + @buffer = root_buffer + @offset_to_buffer = 0 + end + protected def to_lookup_hash to_lookup_hash { |elem| elem } end From 0bb29c36c8a93402a59d3a1736e7760f573bf5ec Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 15 Dec 2020 15:35:28 -0300 Subject: [PATCH 2/3] Array: optimize unshift --- spec/std/array_spec.cr | 16 ++++++++ src/array.cr | 87 ++++++++++++++++++++++++------------------ 2 files changed, 66 insertions(+), 37 deletions(-) diff --git a/spec/std/array_spec.cr b/spec/std/array_spec.cr index ccb979394765..797d66c01ad5 100644 --- a/spec/std/array_spec.cr +++ b/spec/std/array_spec.cr @@ -1554,6 +1554,22 @@ describe "Array" do a.should eq [3, 1, 2] end + it "unshifts one elements three times" do + a = [] of Int32 + 3.times do |i| + a.unshift(i) + end + a.should eq([2, 1, 0]) + end + + it "unshifts one element multiple times" do + a = [1, 2] + (3..100).each do |i| + a.unshift(i) + end + a.should eq((3..100).to_a.reverse + [1, 2]) + end + it "unshifts multiple elements" do a = [1, 2] a.unshift(3, 4).should be(a) diff --git a/src/array.cr b/src/array.cr index d0478817ae94..2b811fefcaf6 100644 --- a/src/array.cr +++ b/src/array.cr @@ -1043,6 +1043,10 @@ class Array(T) # a.insert(-1, "z") # => ["x", "a", "y", "b", "c", "z"] # ``` def insert(index : Int, object : T) + if index == 0 + return unshift(object) + end + if index < 0 index += size + 1 end @@ -1051,15 +1055,9 @@ class Array(T) raise IndexError.new end - # See if we can move backwards if we have space - if index == 0 && @offset_to_buffer > 0 - shift_buffer_by(-1) - @buffer.value = object - else - check_needs_resize - (@buffer + index + 1).move_from(@buffer + index, size - index) - @buffer[index] = object - end + check_needs_resize + (@buffer + index + 1).move_from(@buffer + index, size - index) + @buffer[index] = object @size += 1 @@ -2197,41 +2195,30 @@ class Array(T) # a.unshift(1) # => [1, "c", "a", "b"] # ``` def unshift(object : T) - if @offset_to_buffer > 0 - shift_buffer_by(-1) - @buffer.value = object - else - check_needs_resize - (@buffer + 1).move_from(@buffer, size) - @buffer[0] = object - end - + # If we have no more room left before the beginning of the array + # we make the array larger, but point the buffer to start at the middle + # of the entire allocated memory. In this way, if more elements are unshift + # later we won't need a reallocation right away. This is similar to what + # happens when we push and we don't have more room, except that toward + # the beginning. + if @offset_to_buffer == 0 + double_capacity_for_unshift + end + + # At this point we are sure @offset_to_buffer is greater than zero + shift_buffer_by(-1) + @buffer.value = object @size += 1 + self end # Prepend multiple values. The same as `unshift`, but takes an arbitrary number # of values to add to the array. Returns `self`. def unshift(*values : T) - # Check if we have enough offset from the buffer to fit the values - if @offset_to_buffer >= values.size - shift_buffer_by(-values.size) - values.each_with_index do |value, i| - @buffer[i] = value - end - @size += values.size - return self - end - - resize_if_cant_insert(values.size) - - move_value = values.size - @buffer.move_to(@buffer + move_value, @size) - - values.each_with_index do |value, i| - @buffer[i] = value + values.reverse_each do |value| + unshift(value) end - @size += values.size self end @@ -2242,7 +2229,7 @@ class Array(T) private def check_needs_resize # We have to compare against the actual capacity in case `@buffer` was moved - return unless @size == remaining_capacity + return unless needs_resize? # If the array is not empty and more than half of the elements were shifted # then we avoid a resize and just move the elements to the left. @@ -2304,6 +2291,10 @@ class Array(T) end end + private def needs_resize? + @size == remaining_capacity + end + def remaining_capacity @capacity - @offset_to_buffer end @@ -2321,6 +2312,28 @@ class Array(T) end end + # Similar to double capacity, except that after reallocating the buffer + # we point it to the middle of the buffer in case more unshifts come right away. + # This assumes @offset_to_buffer is zero. + private def double_capacity_for_unshift + resize_to_capacity_for_unshift(@capacity == 0 ? 3 : (@capacity * 2)) + end + + private def resize_to_capacity_for_unshift(capacity) + @capacity = capacity + half = @capacity // 2 + + if @buffer + @buffer = root_buffer.realloc(@capacity) + @buffer.move_to(@buffer + half, @capacity - half) + @buffer.clear(half) + else + @buffer = Pointer(T).malloc(@capacity) + end + + shift_buffer_by(half) + end + private def resize_if_cant_insert(insert_size) # Resize if we exceed the remaining capacity. # `remaining_capacity - @size` is the actual number of slots we have From 8b6a60fd99fbec48feac50f6c4f9903456ba5d2d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 15 Dec 2020 17:05:07 -0300 Subject: [PATCH 3/3] Fix Array debug info --- etc/lldb/crystal_formatters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/etc/lldb/crystal_formatters.py b/etc/lldb/crystal_formatters.py index 856dea7c4f55..fcbc7875c543 100644 --- a/etc/lldb/crystal_formatters.py +++ b/etc/lldb/crystal_formatters.py @@ -11,7 +11,7 @@ def update(self): self.valobj = self.valobj.Dereference() self.size = int(self.valobj.child[0].value) self.type = self.valobj.type - self.buffer = self.valobj.child[2] + self.buffer = self.valobj.child[3] def num_children(self): size = 0 if self.size is None else self.size