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

add an optimization for immutable structs without type arguments #1

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

Conversation

KristofferC
Copy link
Owner

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2020

Codecov Report

Merging #1 into master will decrease coverage by 6.78%.
The diff coverage is 74.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
- Coverage   94.52%   87.73%   -6.79%     
==========================================
  Files           1        1              
  Lines          73      106      +33     
==========================================
+ Hits           69       93      +24     
- Misses          4       13       +9     
Impacted Files Coverage Δ
src/LazilyInitializedFields.jl 87.73% <74.35%> (-6.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d74d92...4003b32. Read the comment docs.

@KristofferC KristofferC changed the title add an optimization for structs without type arguments add an optimization for immutable structs without type arguments Oct 5, 2020
@KristofferC
Copy link
Owner Author

So this seems to improve the performance when using non-lazy field and slightly decrease performance when using the lazy field. Interesting.


# 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

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

src/LazilyInitializedFields.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Owner Author

Note that the proper way of doing this is to have support in julia for declaring individual fields as immutable, which is JuliaLang/julia#9448 (comment).

I heard that JuliaLang/julia#37847 gets us a bit on the way to that.

@KristofferC
Copy link
Owner Author

Another question, is it worth adding @ifdebug or is @static if isdebug() good enough?

Comment on lines +305 to +306
For immutable structs without type parameters we do an optimization to keep the
struct mutable and define it as
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

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

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.

@DilumAluthge
Copy link
Contributor

DilumAluthge commented Dec 24, 2021

Note that the proper way of doing this is to have support in julia for declaring individual fields as immutable, which is JuliaLang/julia#9448 (comment).

I heard that JuliaLang/julia#37847 gets us a bit on the way to that.

@KristofferC Now that Julia master allows annotating mutable struct fields as const (JuliaLang/julia#43305), will that feature be useful for the implementation of this PR?

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.

5 participants