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

Abstract types in structs #27

Open
csimal opened this issue May 6, 2022 · 2 comments
Open

Abstract types in structs #27

csimal opened this issue May 6, 2022 · 2 comments
Labels
enhancement New feature or request performance Should make things go faster
Milestone

Comments

@csimal
Copy link

csimal commented May 6, 2022

The way Starlight.jl's datatypes are currently defined with abstractly-typed fields, e.g. AbstractFloat in

mutable struct Clock
  started::Base.Event
  stopped::Bool
  freq::AbstractFloat
  Clock() = new(Base.Event(), true, 0.01667) # 60 Hz
end

is problematic, as it actually prevents the Julia compiler from working it's performance magic. See this part of the manual.
The problem is that this actually tells the Julia compiler that this field has to be able to hold any subtype of AbstractFloat, so the compiler has to box it, meaning it can't optimize on the actual type of freq at runtime.

The correct way to do this is to use parametric types :

mutable struct Clock{T<:AbstractFloat}
  started::Base.Event
  stopped::Bool
  freq::T
  Clock() = new(Base.Event(), true, 0.01667) # 60 Hz
end

This way, the concrete type of freq is known to the compiler at runtime, so it can optimize on it.


As an aside, a better way to define default fields values is to use Parameters.jl :

@with_kw mutable struct Clock{T<:AbstractFloat}
  started::Base.Event = Base.Event()
  stopped::Bool = true
  freq::T = 0.01667 # 60 Hz
end

This has the advantage that it automatically defines constructors with keyword arguments, so the following snippets are all valid,

julia> Clock()
Clock{Float64}
  started: Base.Event
  stopped: Bool true
  freq: Float64 0.01667

julia> Clock(freq=1/30)
Clock{Float64}
  started: Base.Event
  stopped: Bool true
  freq: Float64 0.03333333333333333

julia> Clock(Base.Event(), false, 1/120)
Clock{Float64}
  started: Base.Event
  stopped: Bool false
  freq: Float64 0.008333333333333333
@jhigginbotham64
Copy link
Owner

Sounds like a small improvement, but a good one. Yeah I'm always looking for ways to make the code more Julian and easier to use. Do you have time to create a PR?

@jhigginbotham64 jhigginbotham64 added enhancement New feature or request good first issue Good for newcomers performance Should make things go faster labels May 6, 2022
@csimal
Copy link
Author

csimal commented May 6, 2022

Good thing I was in the zone: #29

@jhigginbotham64 jhigginbotham64 removed the good first issue Good for newcomers label May 10, 2022
@jhigginbotham64 jhigginbotham64 added this to the v0.3.0 milestone May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Should make things go faster
Projects
None yet
Development

No branches or pull requests

2 participants