Skip to content
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

type stability and compile time improvements #480

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rafaqz
Copy link

@rafaqz rafaqz commented Feb 1, 2022

A grab bag of changes to improve compile time:

  • type stable fields in e.g. Sync and Async, use of empty strings instead of nothing
  • explicitly typed Dict
  • less use of the js"" macro
  • function barriers where there is type instability

Copy link
Member

@twavv twavv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about how backwards compatible these changes are. Will the break anyone using WebIO? Definitely check with Interact.jl (make sure all those tests pass with the dev version of WebIO). I'd also like to see what impacts some of these changes make.

Comment on lines +80 to +85
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit awkward to me (to have a type parameter that's an array). Can you benchmark the changes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Is it possible to register a non-concrete-Observable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rafaqz
Copy link
Author

rafaqz commented Feb 4, 2022

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 slider under one second. Nothing sensational, but this is death by 1000 cuts... it takes all 5 PRs to get slider from 15s to 1s...

@halleysfifthinc
Copy link
Contributor

Bump. Could we get this rolling? With #476 / #478, I think there are substantial enough changes to warrant a minor release, which would be breaking since WebIO is pre v1. Backwards compatibility wouldn't need to be guaranteed in that case.

@rafaqz
Copy link
Author

rafaqz commented Oct 31, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants