type stability and time to first widget improvements

[rafaqz]

This PR aims to improve the load time of widgets.

TTF Interact.slider(1:10) is currently 15 seconds on my machine, after this PR -and the AssetRegistry PR- it is down to 5 3.

Hopefully it can be 1 second or less.

There are some problems with the design of AssetRegistry.jl and Wigets.jl that exacerbate the current load time.

First, using JSON3 instead of JSON in AssetRegistryjl is a huge improvement for all widgets - slider(1:10) in about 7 seconds.

JuliaGizmos/AssetRegistry.jl#15

Second, because backends/themes are passed to all methods in InteractBase, but they are stored in a type unstable Vector, type instability propagates everywhere. Using the type of AbstractBackend and WidgetTheme instead of the constructed backend/theme means most methods wont specialise unless we specifically need them to, and the vector is Vector{Type}. This combined with fixing the type stability of various uses of Dict brings slider in InteractBase.jl down to 5 seconds, and closer to full type stability. This change is remeoved so as not to be so breaking, and removing most of the unstable gettheme calls in InteractBase comes close anyway.

The last (I think) major problem is that the eltype of observables is known from the passed in range (e.g. Int64 above) but it's lost, and has to be detected again in an unstable way in the Widget constructor. We should as much as possible propagate those types throughout. Is there a reason it is detected again from output_obs ?

This will be a breaking change, and needs companion PRs in at least InteractBase.jl and Interact.jl. I'm writing all of these simultaneously, as making the whole ecosytem faster is the goal.

Edit: moving to always passing the eltype parameter S to Widget{T,S} brings the TTFS down to 3 seconds !! This also seems to be backwards compatible, just using one type parameter or passing nothing as the second type parameter triggers the original behaviour.

This was actually solvable with a few tweaks to the constructor methods to keep type stability, and is no longer breaking.

Much of the remaining performance improvments seem to be in WebIO.jl

[piever]

That's interesting, I had no idea that somehow it was better to use types rather than instances for this. OTOH, this means the change is breaking (as I imagine the old Interact / InteractBase will try to set an instance as backend rather than a type).

I imagine we should bump to version 0.7 to reflect the breakage.

[rafaqz]

Yeah, types dont specialise methods they pass through unless Type{T} is used. And the vector is stable. But actually this change is probably not needed now. Previously the theme was checked in every method, meaning constant unstable method dispatch. But I changed the code to just get it once and pass it through, which will mean there is only the first unstable function call. I will see how it goes changing it back.

[rafaqz]

Ok I reverted this change. It does still seem like a little more compilation, but with basically no calls to gettheme and all the other changes here, in AssetRegistry.jl and WebIO.jl I'm still getting 3 seconds to a slider.

[rafaqz]

@piever can you approve the test run? This is ready to go.

[piever]

Thanks for finalizing this (latency improvement is pretty impressive!) and sorry about the annoyance that you can't run tests just by pushing, not sure if there's an easy way to allow you to do that...

It seems like this has some issues with @manipulate. Maybe you don't see them locally because this needs the InteractBase PR to fully work. If it's easy to make it fully backward compatible (so tests pass with currently released InteractBase version), I guess that would be ideal.

If that's not possible, I imagine a possible release strategy could be:

• bump minor version number in Widgets (so currently released InteractBase does not break)
• check out the InteractBase PR in Widgets CI to make sure tests pass
• release Widgets
• update Widgets dep in InteractBase PR and release InteractBase
• revert checking out the InteractBase PR during Widgets CI

[rafaqz]

Yeah I noticed that too after pushing this. I'll look for a way to do this where InteractBase.jl works as is.

[rafaqz]

Ok this should be fine with InteractBase.jl. I found some simpler ways to get some of the type stability benefits without any breaking changes.

It's more like 4 seconds load time, not such a huge improvement here, mostly in the AssetRegistry PR. But type stability fixes to WebIO will help a bit more, and a few in InteractBase.jl

[piever]

This seems like a less invasive approach, I've left a few minor comments which should be straightforward to address. There's one thing I'd like to understand better though. When you say

Previously the theme was checked in every method, meaning constant unstable method dispatch. But I changed the code to just get it once and pass it through, which will mean there is only the first unstable function call.

how exactly does that word? Is it just fixed in InteractBase by preferring getclass(theme, args...) over getclass(args...)?

[rafaqz]

Yes, passing theme through seems better because gettheme() isn't type stable, so we are dispatching on a type not known at runtime. This is all hard to lock down exactly because timings are subject to noise (no @btime), but the profiles look better to me.

[rafaqz]

Ok this should be good to merge