Skip to content

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jul 23, 2023

Closes #36235.

The map! docstring specifies:

Like map, but stores the result in destination rather than a new collection. destination must be at least as large as the smallest collection.

However, we're neither boundschecking the destination container, nor using the smallest input collection:

# should throw
julia> map!(identity, [0], [1,2])
1-element Vector{Int64}:
 1

# uses undefined values
julia> map!(+, [0,0], [1,2], [3], [4])
2-element Vector{Int64}:
           8
 10187977346

This PR corrects that.

Will need some careful benchmarking, maybe add some @inbounds to map! calls and/or outlining the throwing.

FWIW, the performance of the example in #30624 (comment) is identical.

@maleadt maleadt added arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug labels Jul 23, 2023
@maleadt
Copy link
Member Author

maleadt commented Jul 23, 2023

@nanosoldier runbenchmarks(ALL, vs=":master")

@maleadt maleadt force-pushed the tb/map!_boundschecks branch from 362067c to e833d4c Compare July 23, 2023 16:41
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@maleadt maleadt force-pushed the tb/map!_boundschecks branch from e833d4c to 9aefacd Compare July 24, 2023 13:50
@maleadt maleadt changed the title Add a couple of boundschecks to map! Make map! correctly handle differently-sized containers. Jul 24, 2023
@maleadt maleadt marked this pull request as ready for review July 24, 2023 14:00
@maleadt maleadt marked this pull request as draft July 24, 2023 14:58
@maleadt
Copy link
Member Author

maleadt commented Jul 24, 2023

Looks like this exposes some other existing issues, e.g. with OffsetArrays:

julia> map!(+, [0, 0, 0, 0], fill(0, -4:-1), fill(0, -4:-1), fill(0, -4:-1))
Int64[]

julia> map!(+, [0, 0, 0, 0], fill(0, -4:-1), fill(0, -4:-1))
4-element Vector{Int64}:
 0
 0
 0
 0

@maleadt maleadt force-pushed the tb/map!_boundschecks branch from 400d905 to e2be645 Compare July 25, 2023 21:45
@maleadt maleadt force-pushed the tb/map!_boundschecks branch from e2be645 to 84acf71 Compare July 26, 2023 16:24
@maleadt
Copy link
Member Author

maleadt commented Jul 26, 2023

@nanosoldier runbenchmarks(ALL, vs=":master")

@maleadt maleadt marked this pull request as ready for review July 26, 2023 19:12
@maleadt
Copy link
Member Author

maleadt commented Jul 26, 2023

The changes in allocations here can be ignored, I think. They come from the additional bounds checks, but only when running with --check-bounds=yes (that the boundscheck is elided for that specific test). The failing test:

include("../../test/testhelpers/OffsetArrays.jl")
using Main.OffsetArrays
R = fill(0, -4:-1, 7:7, -6:-6)
B = OffsetArray(reshape(1:24, 4, 3, 2), -5, 6, -7)
@allocated maximum!(R, B)

1264 bytes on master, 1248 on this PR, but 1408 bytes when forcing bounds checks.

@maleadt
Copy link
Member Author

maleadt commented Jul 26, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@adienes
Copy link
Member

adienes commented Oct 2, 2023

was the attempt here successful? is there anything I can do to assist? e.g. particular benchmark regressions

@maleadt
Copy link
Member Author

maleadt commented Oct 2, 2023

Sadly not, the performance regressions are real, but I didn't have the time to look into them. Feel free to take this up and create your own PR, if you want.

@LilithHafner
Copy link
Member

Closing as abandoned, feel free to re-open if you'd like to pick it back up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrays [a, r, r, a, y, s] bugfix This change fixes an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

map! resulting in undefined values

4 participants