api improvements

[SimonDanisch]

• add ignore_equal_values to Observable constructor, which implements https://github.com/JuliaGizmos/Observables.jl/pull/79 without adding a new type
• remove observe_changes
• add PriorityObservable from Makie without adding a new type (from PriorityObservable.jl). One can now simply add on(f, priority::Int=...), to register a callback with priority.
• add on(f, obs; update=true), to call f(obs[]) immediately.
• improve compile times quite a bit
• remove Task based, Channel based observable API

Based on sd/compile-time not for the best reasons, besides it being a bit easier

[yha]

Some thoughts:

• The boolean use_priority field seems unnecessary. If users simply don't pass priority values (resulting in the default priority 0 for everything), and don't use the Consume type, you automatically get the "normal" behavior. This redundancy in the interface allows the user to contradict themselves by setting use_priority=false and also setting priorities when calling on (and the implementation just ignores the priorities and sets them to 0 in this case). I would just remove this field, and the branch in on.
• Why is listeners a Vector{Pair{Int,Any}} and not a Dict{Int,Any}?
• Having a boolean ignore_equal_values seems unnecessarily restrictive, and it drops the support (currently in observe_changes) for specifying the relevant "equality" predicate. That field can more generally be the relevant predicate instead, so it would default to Returns(false) (always update), and you can set it to isequal to get the current PR behavior, or to other values. I also think this could be a type parameter rather than a field to get more performance. That would mean you can't change it in an existing Observable, but is that really needed? (Incidentally, why is Observable mutable?)

[SimonDanisch]

The boolean use_priority field seems unnecessary

True, good catch! I started with a different implementation where it was needed and forgot to remove now that it isn't needed!

Why is listeners a Vector{Pair{Int,Any}} and not a Dict{Int,Any}

Because keys occur multiple times! Also, iterate + findfirst are the main operations we need to use, so vector fits well.

Having a boolean ignore_equal_values seems unnecessarily restrictive

I wanted to make it as simple as possible for the basic use case, and not make the type more complicated with more type parameters. I'm not 100% sure how much faster 1 type parameter compiles faster than two, but alone the complexity increase to store them concretely in other structs seems not worth it. Especially, since one can easily update with some other comparison method without this flag!

[yha]

Because keys occur multiple times!

Right, I meant that a key would map to a vector of listeners. But I realize now that the idea is to maintain a sorted list of keys, so you would actually want a SortedDict which is an extra dependency. So a vector of pairs makes sense. But maybe the insertion should be

idx = searchsortedfirst(ls, priority; by=first, rev=true)
insert!(ls, idx, priority => f)

since it makes it a bit clearer that it's about maintaining a sorted list? Might just be a matter of taste.

I'm not 100% sure how much faster 1 type parameter compiles faster than two, but alone the complexity increase to store them concretely in other structs seems not worth it.

I don't follow. Why would you need to store the parameters in other structs? And is the objection to having a predicate instead of a boolean, or just to having as a type parameter?

[SimonDanisch]

But maybe the insertion should be

Yes, agreed! :)

Why would you need to store the parameters in other structs?

Well, if you want to concretely put any observable in a struct you'll need to do:

struct OtherStruct{T, COMP}
    obs::Observable{T, COMP}
end

I guess one could also do:

struct OtherStruct{O <: Observable}
    obs::O
end

But that doesn't work for all use cases.

[timholy]

Can you give a practical use case where you'd want an equality-comparison operator different from ==/isequal? I'm all in favor of flexiblity if it buys you something, but in this case your choices are (1) make the equality-check field a type-parameter, leading to more specialization (and more latency) for methods that take Observables as arguments, (2) allow it to be ::Any and thus use runtime dispatch. Hard-coding the comparison operator has neither of these disadvantages, instead it has the disadvantage of being less flexible.

[yha]

I never used observe_changes with a different equality operator, but I can imagine it might be useful with approximate equality, e.g. when tracking mouse movement. (But of course, this can also be done manually on on) My thinking was that using an equality operator is both simpler code and more general, but maybe it's not worth the dynamic dispatch.

[yha]

Also, I'm not sure observe_changes should be dropped. It's still useful as a primitive operation by itself, to do things like

map(f, obs1, obs2, observe_changes(obs3))

instead of the more cumbersome

map(f, obs1, obs2, map(identity, obs3; change_observable=true))

I don't know how common this is, though. I've used this pattern, but it's a very small sample.

[SimonDanisch]

@timholy I went through all @nospecialize constructs etc and measured whether they're necessary or not, and removed/added some.

This is now fastest for Makie itself, but let me know if that doesn't work well with some of your Projects! I think we can merge this now and release a breaking version. I'd like to merge it soon, since we can still tweak a few things in patch releases ;)

I've also brought back most of the features I removed, as long as they didn't have compile time problems. I'm still removing the weird async features I added at some point, since I don't think anyone has ever used them and I'm not really sure about their implementation. This PR shaves off around 17% of the ttfp at this point.

[timholy]

Git history is now really messy, with lots of commit names that aren't very useful (e.g., "does this help??"). Please use squash!