-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Use shared pre-computation for by
and perm
orderings.
#52033
base: master
Are you sure you want to change the base?
Conversation
Fabulous idea!
Couldn't you fix that by using "traits" to declare that a type supports the new API? struct Preparable end
struct NotPreparable end
preparable(::Ordering) = NotPreparable()
preparable(::ReverseOrdering) = Preparable()
preparable(::By) = Preparable()
preparable(::Perm) = Preparable() If you need |
We could do that, or we could say the "trait" is to define lt(::MyNewPreparableOrdering, x, y) = lt_prepared(o, prepare(o, x), prepare(o, y)) In order of verbosity:
Full traits also allow algorithmic dispatch based on the presence of preparability, but I think that is actually something we do not want to support because I want supporting preparation to remain totally free (i.e. never a regression vs not supporting preparation). If you have a comparison that is 90% lt and 10% preparation, it still makes sense to share the preparation and I don't want to dispatch based on the existence of preparation for that reason. Unfortunately, preparability is only recursive if each Ordering supports it explicitly.
|
The API this adds is internal, the downside in the OP is no longer present, and I'd like to merge this before working on sortperm performance so that that stuff cleanly generalizes to by orders as well. So I'm inclined to merge this unless anyone objects. |
How many times
by
is calledRuntime performance tests
Something to maybe consider doing later is marking this
lt_prepared
API public.A downside of this PR is that it makes comparing with an
Ordering
subtype that defines neitherlt
norlt_prepared
a stack overflow error instead of a method not found error. This could happen of someone definesMyOrder <: Ordering
andlt(::MyOrder, ::MyType, ::MyType)
and then callslt(o::MyOrder, 1, 2)
. It's possible to remove that downside by replacingwith
But that makes the (internal)
lt_prepared
API more cludgy to use.