improve performance of searchsorted(range, lt=<)

[aplavin]

No behavior change, just a performance improvement: searchsorted(range, lt=<) now performs exactly as fast as searchsorted(range).

Passing lt=< allows searchsorted to find floating point values such as -0.0, so it's useful to make it fast - see #44102 (comment).

[LilithHafner]

I think a dispatch-based approach would be more idiomatic, concise, and extensible, unless there are performance issues:

_ord(lt::typeof(isless), by, order::Ordering)        = _by(by, order)
_ord(lt,                 by, order::ForwardOrdering) = _by(by, _lt(lt))
_ord(lt,                 by, order::ReverseOrdering) = reverse(_by(by, _lt(lt)))
_ord(lt,                 by, order::Ordering) =
    error("Passing both lt= and order= arguments is ambiguous; please pass order=Forward or order=Reverse (or leave default)")
_by(by, order::Ordering) = By(by, order)
_by(::typeof(identity), order) = order
_lt(lt) = Lt(lt)
_lt(::typeof(isless)) = ForwardOrdering

[aplavin]

@LilithHafner yeah I guess dispatch is cleaner, here I followed the existing code just below - it uses ifs. Will update to use dispatch!

[aplavin]

Also added tests, these options weren't really tested for ranges as I can see.
Should be ready, if tests don't fail.

[aplavin]

Tests fail but failures are irrelevant, so idk how to proceed further.

[LilithHafner]

There's a whitespace test failure—that is almost always relevant

[aplavin]

The remaining failure is irrelevant I guess?

[aplavin]

Gentle bump...

[vtjnash]

If you need a Union like DirectOrdering, usually that is a sign that something is designed poorly, and a bit buggy in edge case. But it appears that all of these types in the Union are concrete, so it is not huge and much less likely to be buggy, but still a little bit concerning design-wise. Is this Union likely to need to grow in the future, and if so, is there something better?

[aplavin]

DirectOrdering already exists and was used in this function signature before. Here, I extended it with two more concrete types.
DirectOrdering is also user for fastpath in sort, with missings and with floats/integers. Sorting missings doesn't play nicely with < anyway, and < on floats is complicated by NaNs, so I guess these two unions will remain different.

I don't foresee any extension to this union. In principle, stuff like isgreater and > could also be supported, but there's no need given the rev argument.

[LilithHafner]

This type gates dispatch to an optimization that relies on a monotonic order over the real numbers. It could be extended to be arbitrarily large to e.g. include By{typeof(atan), ForwardOrdering}.

This pr optimizes
searchsorted(range, lt=<)
That further extension would optimize
searchsorted(range, by=atan)

A more general solution would be an ismonotonic trait.

All this is a tradeoff between runtime performance on easy edge cases vs. compile times and sysimg size. It's unfortunate that it is so difficult to instrument compile time and sysimg size regressions.

[aplavin]

bump!

[LilithHafner]

I refrained from merging because of

All this is a tradeoff between runtime performance on easy edge cases vs. compile times and sysimg size [and source code complexity]. It's unfortunate that it is so difficult to instrument compile time and sysimg size regressions.

I'm not necessarily opposed to someone else merging this, I just want to take responsibility for those hard to quantify regressions in exchange for a performance win on such a small edge case.

[aplavin]

Arguably, < is more natural as searchsorted comparison than isless. The latter (default) doesn't find == elements such as -0.0/+0.0:

julia> searchsorted(-1000.0:1:1000, -0.0)
1001:1000

julia> searchsorted(-1000.0:1:1000, -0.0; lt=<)
1001:1001

So I think it's important to make < searches performant as well.

That's exactly what this PR does.
Other potential orderings like by=atan mentioned above are surely much less common. While traits ismonotonic/islinear/... would be useful for many usecases including this one, they would be a much larger change compared to this PR.

[aplavin]

bump

[aplavin]

bump...

[LilithHafner]

Looking at this again after a while, I'm remembering that passing lt=< to a sorting function is an error (UB on older versions of Julia) so special casing an optimization for it's (non UB) use in searchsorted seems even less appealing to me.

[vtjnash]

That is good input, but I think I am okay with that minor risk here

[aplavin]

@LilithHafner my only motivation here is to have performant search that can actually find equal values (like 0 and -0). For me, using and optimizing lt=< in searchsorted seems the most straightforward way to get that – but if there are even cleaner approaches, I'm all for them as well.

[hyrodium]

Is there any chance to have this PR to be backported to v1.10?

[oscardssmith]

No. We don't backport performance only PRs.