Skip to content
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.

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

Faster searchsorted methods for range and array inputs #38527

Closed

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Nov 22, 2020

For searching through an AbstractRange, the fastest one can use searchsortedfirst to evaluate multiple values currently is:

function foo(r, x)
   i = similar(x, keytype(r))
   @inbounds for ci in CartesianIndices(x)
       i[ci] = searchsortedfirst(r,x[ci])
   end
   return i
end
r = 0x00:0x02:0xff;
x = rand(UInt8, 1000, 1000);
@btime foo($r, $x)
  6.159 ms (2 allocations: 7.63 MiB)
1000×1000 Matrix{Int64}: ...

This PR provides the following, which only checks for one-based indexing and the length of r once, given they are expensive. It can also be preallocated:

@btime searchsortedfirst($r, $x)
  3.193 ms (2 allocations: 7.63 MiB)
1000×1000 Matrix{Int64}: ...
i = similar(x, keytype(r));
@btime searchsortedfirst!($i, $r, $x) 
  2.830 ms (0 allocations: 0 bytes)
1000×1000 Matrix{Int64}: ...

I didn't extend the methods for searching through AbstractVectors, as I didn't see a performance limitation with using those looped through values

@mcabbott
Copy link
Contributor

Is it necessary to require one-based indexing at all? Perhaps + one(keytype(a)) can just be + firstindex(a), or does this go wrong somewhere?

@dkarrasch dkarrasch added collections Data structures holding multiple items, e.g. sets performance Must go faster labels Nov 22, 2020
@IanButterworth
Copy link
Member Author

Maybe! Would zero become firstindex(a) - 1?

I didn't feel confident enough to try to handle non one based indexed ranges. I assume that it would require adding test cases for stuff out in the ecosystem that I'm unaware of

@mcabbott
Copy link
Contributor

The only example I know if is the axes of OffsetArrays, which behave like this:

julia> OffsetArray('a':'z', -10)
'a':1:'z' with indices -9:16

julia> axes(ans, 1)
OffsetArrays.IdOffsetRange(-9:16)

julia> axes(ans, 1)
OffsetArrays.IdOffsetRange(-9:16)

The general idea here is similar to JuliaMath/IntervalSets.jl#63, in which it wasn't so hard to allow offsets. But that can depend on the package for tests. I think there is a (simplified?) implementation of this loaded for tests, e.g. https://github.com/JuliaLang/julia/blob/master/test/offsetarray.jl

@KristofferC
Copy link
Member

Is the addition of the new API (the bang versions of searchsortedxxx) necessarily coupled with the rest of this PR?

@IanButterworth
Copy link
Member Author

Not necessary, but logical I think. I think it would be odd to add the array-based methods without allowing preallocation.

Do you think the PR needs to be split?

base/sort.jl Outdated Show resolved Hide resolved
base/sort.jl Outdated Show resolved Hide resolved
base/sort.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Apr 6, 2021

While looking into this, I sought to understand the code, and realized there was a broken case resulting as the code had been slowly diverging over the years. I think this can be replaced by #40364 and #40358, and achieves the same performance objective without needing these new methods

@IanButterworth
Copy link
Member Author

Nice. Thanks!

vtjnash added a commit that referenced this pull request Apr 6, 2021
@IanButterworth
Copy link
Member Author

Closing given superseded by #40364 and #40358

@IanButterworth IanButterworth deleted the ib/faster_searchsorted branch April 8, 2021 01:09
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants