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.

Sign up for GitHub

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

add an optimization for immutable structs without type arguments #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add an optimization for immutable structs without type arguments #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
15524ba
add an optimization for structs without type arguments
KristofferC Oct 5, 2020
f62806b
fix for pre nightly
KristofferC Oct 5, 2020
6a77b22
some stupid convert stuff
KristofferC Oct 6, 2020
4003b32
Update src/LazilyInitializedFields.jl
KristofferC Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 46 additions & 13 deletions page/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,40 +180,60 @@ Body::Union{Nothing, Int64}

\begin{:section, title="Caveats"}

When applying `@lazy` to a non-mutable struct, the standard way of mutating it
via `setproperty!` (the `f.a = b` syntax) is disabled. However, the struct is
still considered mutable to Julia and the `setproperty!` can be bypassed:
When applying `@lazy` to a non-mutable struct that has type parameters, the
standard way of mutating it via `setproperty!` (the `f.a = b` syntax) is
disabled. However, the struct is still considered mutable to Julia and the
`setproperty!` can be bypassed:

```julia-repl
julia> @lazy struct Foo
a::Int
@lazy b::Int
julia> @lazy struct Baz{T}
a::T
@lazy b::Float64
end

julia> f = Foo(1, uninit)
Foo(1, uninit)
julia> b = Baz(1, uninit)
Baz{Int64}(1, uninit)

julia> f.a = 2
ERROR: setproperty! for struct of type `Foo` has been disabled
julia> ismutable(b)
true

julia> b.a = 2
ERROR: setproperty! for struct of type `Baz` has been disabled
[...]

julia> setfield!(f, :a, 2)
julia> setfield!(b, :a, 2)
2

julia> f.a
julia> b.a
2
```

The fact that the struct is considered mutable by Julia also means that it will
no longer be stored inline in cases where the non `@lazy` version would:

```julia-repl
julia> isbitstype(Foo)
julia> isbitstype(Baz{Int})
false
```

This has an effect if you would try to pass a `Vector{Foo}` to e.g. C via `ccall`.

However, for immutable structs without type parameters, the type can be kept immutable
(it's posible to do an optimization in this case):

```julia-repl
julia> @lazy struct Foo
a::Int
@lazy b::Int
end

julia> f = Foo(1, uninit)
Foo(1, uninit)

julia> setfield!(f, :a, 2)
ERROR: setfield! immutable struct of type Foo cannot be changed
```

\end{:section}

\begin{:section, title="Implementation"}
Expand Down Expand Up @@ -282,4 +302,17 @@ transformations that checks that the field being manipulated is lazy (via
`islazyfield`) and converts `getproperty` and `setproperty!` to `getfield` and
`setfield!`.

For immutable structs without type parameters we do an optimization to keep the
struct mutable and define it as
Comment on lines +305 to +306
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like it would always generate worse code than just making it mutable, so calling it an optimization seems misleading. Perhaps say something like "this will make a larger, slower, fragmented layout in order to make just those fields mutable"?

Copy link
Owner Author

@KristofferC KristofferC Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Having those fields non mutable is a feature. The fact that is not faster is a compiler performance bug. It's up to you to make it fast ;).

Copy link
Contributor

Choose a reason for hiding this comment

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

You're semantically requiring it to be slower though, so there's not much the compiler can do to undo that (other than convert it back to a mutable struct, if you're very lucky). You requiring the computer to do more work at runtime.

Copy link
Owner Author

Choose a reason for hiding this comment

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

(other than convert it back to a mutable struct, if you're very lucky)

It can do whatever it wants as long as setfield on the fields that are suppose to be immutable, errors. But we are all just beating around the bush that we don't have

mutable struct Foo
    @const x::Int
    @const y::Int
    z::Int
end

Copy link

Choose a reason for hiding this comment

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

It can do whatever it wants as long as setfield on the fields that are suppose to be immutable, errors.

Because Ref (or Box) is mutable, it needs its own identity (must be GC'able independently) and so it cannot be allocated inline in struct Foo (AFAICT). So you're stuck with n_lazy_fields small allocations instead of one bigger allocation, which was @vtjnash's point.

Copy link
Owner Author

@KristofferC KristofferC Oct 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but the current implementation is the only way to get the desired semantics. Whenever there is a better way, I'll switch to it. And I probably won't merge this PR until there is one.


```
struct Foo
a::Int
b::Box{Union{Uninitialized, Int}}
end
```

and make the macros, `getproperty` and `setproperty!` unpack and pack the value
into the `Box` (which is similar to a `Ref`).

\end{:section}
69 changes: 63 additions & 6 deletions src/LazilyInitializedFields.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,50 @@ Base.show(io::IO, u::Uninitialized) = print(io, "uninit")
# islazyfield(::Type{Foo}, s::Symbol) = s === :a || s === :b
function islazyfield end

# For immutable types without type parameters we create a Box
# for the lazy fields instead of making the whole struct mutable
# https://github.com/JuliaLang/julia/issues/35053
mutable struct Box{T}
x::T
end
Base.getindex(b::Box) = b.x
Base.setindex!(b::Box, v) = b.x = v
Base.convert(::Type{Box{T}}, x) where {T} = Box{T}(x)
Base.show(io::IO, b::Box) = show(io, b.x)


# This is a replication of the Nothing and Missing conversion functionality from Base.
if isdefined(Base, :typesplit)
nonuninittype(::Type{T}) where {T} = Base.typesplit(T, Uninitialized)
else
nonuninittype(::Type{T}) where {T} = Core.Compiler.typesubtract(T, Uninitialized)
end
promote_rule(T::Type{Uninitialized}, S::Type) = Union{S, Uninitialized}
function promote_rule(T::Type{>:Uninitialized}, S::Type)
R = nonuninittype(T)
R >: T && return Any
T = R
R = promote_type(T, S)
return Union{R, Uninitialized}
end

function nonuninittype_checked(T::Type)
R = nonuninittype(T)
R >: T && error("could not compute non-uninit type")
return R
end

# These are awful
Base.convert(::Type{T}, x::T) where {T>:Uninitialized} = x
Base.convert(::Type{T}, x) where {T>:Uninitialized} = convert(nonuninittype_checked(T), x)
Copy link

Choose a reason for hiding this comment

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

Out of curiosity, I'm guessing you didn't use

Suggested change
Base.convert(::Type{T}, x) where {T>:Uninitialized} = convert(nonuninittype_checked(T), x)
Base.convert(::Type{Uninitialized}, x) = convert(nonuninittype_checked(T), x)

because of method ambiguities?

At that point, why don't you just write the constructor for parametric types, like you suggested?

Copy link
Owner Author

@KristofferC KristofferC Oct 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

At that point, why don't you just write the constructor for parametric types, like you suggested?

Because I don't know how to write it in a way that will work properly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Union and Nothing has these type of wide conversion methods and in case we have a

@lazy struct Foo
    @lazy a::Union{Nothing, Float32}
end
Foo(1)

these conversions seem to be needed

Base.convert(::Type{T}, x) where T>:Union{Uninitialized, Nothing} = convert(nonuninittype_checked(T), x)
Base.convert(::Type{T}, x) where T>:Union{Uninitialized, Missing} = convert(nonuninittype_checked(T), x)
Base.convert(::Type{T}, x) where T>:Union{Uninitialized, Missing, Nothing} = convert(nonuninittype_checked(T), x)
# Ambiguity resolution
Base.convert(::Type{T}, x::T) where T>:Union{LazilyInitializedFields.Uninitialized, Nothing} = x
Base.convert(::Type{T}, x::T) where T>:Union{LazilyInitializedFields.Uninitialized, Missing} = x
Base.convert(::Type{T}, x::T) where T>:Union{Nothing, Missing, LazilyInitializedFields.Uninitialized} = x


struct NonLazyFieldException <: Exception
T::DataType
Expand All @@ -96,6 +140,8 @@ end
Base.showerror(io::IO, err::UninitializedFieldException) =
print(io, "field `", err.s, "` in struct of type `$(err.T)` is not initialized")

@inline _setfield!(x, s::Symbol, v) = !isimmutable(x) ? setfield!(x, s, v) : getfield(x, s)[] = v
@inline _getfield(x, s::Symbol) = !isimmutable(x) ? getfield(x, s) : getfield(x, s)[]

"""
init!(a, s::Symbol)
Expand All @@ -104,7 +150,7 @@ Function version of [@init!](@ref).
"""
@inline function init!(x::T, s::Symbol, v) where {T}
islazyfield(T, s) || throw(NonLazyFieldException(T, s))
return setfield!(x, s, v)
return _setfield!(x, s, v)
end

_check_setproperty_expr(expr, s) =
Expand Down Expand Up @@ -156,7 +202,7 @@ Function version of [@isinit](@ref).
"""
@inline function isinit(x::T, s) where {T}
islazyfield(T, s) || throw(NonLazyFieldException(T, s))
!(getfield(x, s) isa Uninitialized)
!(_getfield(x, s) isa Uninitialized)
end
"""
@isinit a.b
Expand Down Expand Up @@ -199,7 +245,7 @@ Function version of [`@uninit!`](@ref).

@inline function uninit!(x::T, s::Symbol) where {T}
islazyfield(T, s) || throw(NonLazyFieldException(T, s))
return setfield!(x, s, uninit)
return _setfield!(x, s, uninit)
end

"""
Expand Down Expand Up @@ -269,6 +315,11 @@ end

function lazy_struct(expr)
mutable, structdef, body = expr.args
has_typevar = expr.args[2] isa Expr && expr.args[2].head === :curly
# We cannot use a box for the lazy value in case we have type
# parameters due to https://github.com/JuliaLang/julia/issues/35053
use_box = !mutable && !has_typevar
expr.args[1] = !use_box
structname = if structdef isa Symbol
structdef
elseif structdef isa Expr && structdef.head === :curly
Expand All @@ -277,11 +328,16 @@ function lazy_struct(expr)
error("internal error: unhandled expression $expr")
end

expr.args[1] = true # make mutable
lazyfield = QuoteNode[]
for (i, arg) in enumerate(body.args)
if arg isa Expr && arg.head === :macrocall && arg.args[1] === Symbol("@lazy")
body.args[i] = macroexpand(@__MODULE__, arg)
# a::Union{A, B} -> a::Union{A, B, Uninitialized}
expanded = macroexpand(@__MODULE__, arg)
if use_box
# Union{A, B, Uninitialized} -> Box{Uninitialized{A, B, Uninitialized}}
expanded.args[2] = :($Box{$(expanded.args[2])})
end
body.args[i] = expanded
name = body.args[i].args[1]
@assert name isa Symbol
push!(lazyfield, QuoteNode(name))
Expand All @@ -301,13 +357,14 @@ function lazy_struct(expr)
function Base.getproperty(x::$(esc(structname)), s::Symbol)
if $(LazilyInitializedFields).islazyfield($(esc(structname)), s)
r = Base.getfield(x, s)
$(use_box ? :(r = r[]) : nothing)
r isa $Uninitialized && throw(UninitializedFieldException(typeof(x), s))
return r
end
return Base.getfield(x, s)
end
end)
if !mutable
if !mutable && !use_box
push!(ret.args, quote
function Base.setproperty!(x::$(esc(structname)), s::Symbol, v)
error("setproperty! for struct of type `", $(esc(structname)), "` has been disabled")
Expand Down
66 changes: 57 additions & 9 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,7 @@ using Test
end
f = Foo{Int}(1, uninit, 2.0, uninit, 3.0)

@lazy struct Mut{T}
a::T
@lazy b::Int
end
m = Mut(1, uninit)

@testset "LazilyInitializedFields" begin
@testset "mutable typevar" begin
@test f.a == 1
@test_throws UninitializedFieldException f.b
@test f.c == 2.0
Expand Down Expand Up @@ -47,11 +41,65 @@ m = Mut(1, uninit)
@uninit! f.c
@test !@isinit(f.c)
@test_throws UninitializedFieldException f.d
end

@test_throws LoadError @macroexpand @lazy a::Int

@lazy struct ImMut{T}
a::T
@lazy b::Int
end
m = ImMut(1, uninit)
@testset "immutable typevar" begin
@test_throws ErrorException m.a = 2
@test_throws ErrorException m.b = 2
end

@lazy struct Boxed
a::Int
@lazy b::Float64
end
box = Boxed(1, uninit)

@lazy struct Boxed
a::Int
@lazy b::Float64
end
box = Boxed(1, uninit)

@testset "Boxed" begin
@test isimmutable(box)
@test Boxed(1, 1).b === 1.0 # test conversion constructor
@test !(@isinit box.b)
@init! box.b = 2
@test @isinit box.b
@test box.b == 2
@uninit! box.b
@test !(@isinit box.b)
end

@lazy struct BoxedUnion
a::Int
@lazy b::Union{Missing, Nothing, Int}
end
boxed_union = BoxedUnion(1, uninit)

@testset "Boxed" begin
@test isimmutable(box)
@test !(@isinit box.b)
if VERSION >= v"1.2"
@test BoxedUnion(1, 2.0).b === 2
else
@test_broken BoxedUnion(1, 2.0).b === 2
end
@init! box.b = 2
@test @isinit box.b
@test box.b == 2
@uninit! box.b
@test !(@isinit box.b)
end

@testset "misc" begin
@test_throws LoadError @macroexpand @lazy a::Int
@test_throws LoadError @macroexpand @lazy struct Bar end
end

doctest(LazilyInitializedFields; manual=false)