type stability and compile time improvements #480
Conversation
| struct Sync{A<:Array} | ||
| imports::A | ||
| Sync(imports::A) where A<:Array = begin | ||
| assets = [ensure_asset(asset) for asset in imports] | ||
| new{typeof(assets)}(assets) | ||
| end |
There was a problem hiding this comment.
This feels a bit awkward to me (to have a type parameter that's an array). Can you benchmark the changes?
There was a problem hiding this comment.
This makes them type stable to access, Array just limits A to only be an Array. A is just a regular type parameter.
| @@ -18,7 +18,7 @@ import Sockets: send | |||
| import Observables: Observable, AbstractObservable, listeners | |||
|
|
|||
| # bool marks if it is synced | |||
| const ObsDict = Dict{String, Tuple{AbstractObservable, Union{Nothing,Bool}}} | |||
| const ObsDict = Dict{String, Tuple{Observable, Union{Nothing,Bool}}} | |||
There was a problem hiding this comment.
Is this correct? Is it possible to register a non-concrete-Observable?
There was a problem hiding this comment.
I'm not sure. It doesn't break anything and improves stability, but I'm not sure what the idea of having AbstractObservable is for, or if there are any of those in the wild.
|
It doesn't break Interact.jl or Blink.jl or anything they use, they're the motivation for this. Maybe other things, I'm not sure... The other PR is more important than this too, this is comparatively minor. If you want to see benchmarks for this, we should merge that and rebase and compare to that base line. After that and the other improvements to InteractBase/Widgets.jl/AssetRegistry.jl, This PR gives another 20% improvement or so getting |
|
I lost some steam on this after fixing Parsers.jl, which was the major source of compile time here. But yes we should clean it up. |
A grab bag of changes to improve compile time:
SyncandAsync, use of empty strings instead ofnothingDictjs""macro