StaticArray improvements (map_with_index!) #3356
Conversation
Nephos
force-pushed
the
feature/better_static_array
branch
from
7d5741a
to
66b7b56
Compare
| @@ -64,7 +64,7 @@ $(O)/all_spec: deps $(SOURCES) $(SPEC_SOURCES) | |||
|
|
|||
| $(O)/std_spec: deps $(SOURCES) $(SPEC_SOURCES) | |||
| @mkdir -p $(O) | |||
| $(BUILD_PATH) ./bin/crystal build $(FLAGS) -o $@ spec/std_spec.cr | |||
| $(BUILD_PATH) ./bin/crystal build $(FLAGS) -o $@ spec/std/static_array_spec.cr | |||
There was a problem hiding this comment.
You can use bin/crystal spec spec/std/static_array_spec.cr to run individual specs.
There was a problem hiding this comment.
yes sorry i remove this one ^^
| @@ -163,6 +163,25 @@ struct StaticArray(T, N) | |||
| self | |||
| end | |||
|
|
|||
| def map | |||
| clone.map! { |e| yield e } | |||
There was a problem hiding this comment.
Instead of clone, wouldn't be better to create the array with the final values using https://crystal-lang.org/api/0.19.2/StaticArray.html#new%28%26block%3AInt32-%3ET%29-class-method ?
This comment applies for #map_with_index's and #reverse.
I am not sure how much performance improvement would be though. You can measure performance using Benchmark.ips
There was a problem hiding this comment.
require "benchmark"
Benchmark.ips do |x|
a = StaticArray[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
x.report("clone") { a.clone }
x.report("new") { typeof(a).new{|i| a[i]} }
end> crystal run b.cr --release
clone 328.13M (± 2.70%) 1.00× slower
new 328.83M (± 2.23%) fastest
> crystal run b.cr --release
clone 328.56M (± 2.36%) fastest
new 325.84M (± 5.56%) 1.01× slower
On big StaticArray I finish with
clone 328.88M (± 3.08%) fastest
new 2.48M (± 0.48%) 132.63× slower
There was a problem hiding this comment.
Regardless of the benchmark, invoking clone is wrong because it invokes clone on each element in the static array. This is fine for numbers, but you don't really want to clone objects inside the array.
I'd do what @bcardiff suggests: create a new static array. These can use the new(&block : Int32 -> T) method (similar to how Array#reverse and Array#map_with_index are implemented)
Nephos
changed the title
| def map_with_index! | ||
| i = 0 | ||
| to_unsafe.map!(size) do |e| | ||
| i += 1 |
There was a problem hiding this comment.
Incrementing i after yield would allow to drop i - 1 part which seems bit odd... just a thought :)
There was a problem hiding this comment.
map! do |e|
res = yield e, i
i += 1
res
end?
Nephos
force-pushed
the
feature/better_static_array
branch
from
4e7a9e1
to
710e297
Compare
Nephos
changed the title
|
@Nephos are you still working on this? |
|
I must resolve the new conflicts but I've nothing more to add in this. |
|
@Nephos thanks for this! Ping me when you resolve the conflicts and we'll merge. |
|
I've to open a new PR, because I got an issue with the history (unable to merge because of unrelated history / I can't just remove & update the branch) |
|
Thanks for clarifying! I was wondering :) |
Add the function
map_with_index!relative to #3354