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

types: lazy initialize the field-types when first needed #31877

Merged
merged 1 commit into from
May 6, 2019

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 29, 2019

For concrete datatypes, we still always initialize the fieldtypes
immediately (so that we can immediately also compute the layout and other
related properties).

Note that this implies that constructing the fieldtype is no longer part of
subtyping, (since construction no longer verifies these conditions), and so
impossible constraints now lead to a computed value of Union{} being given
for the type of that field, instead of throwing an error when trying to
allocate the type.

It might be possible to instead store this error, and rethrow it when someone
tries to construct a (partially initialized) copy of the type and/or when the
user (but not inference or introspection tools) tries to examine the fieldtype
of the object. But I think this extra complexity (and additional failure cases
to consider) isn't worthwhile to add.

For concrete datatypes, we still always initialize the fieldtypes
immediately (so that we can immediately also compute the layout and other
related properties).

Note that this implies that constructing the fieldtype is no longer part of
subtyping, (since construction no longer verifies these conditions), and so
impossible constraints now lead to a computed value of `Union{}` being given
for the type of that field, instead of throwing an error when trying to
allocate the type.

It might be possible to instead store this error, and rethrow it when someone
tries to construct a (partially initialized) copy of the type and/or when the
user (but not inference or introspection tools) tries to examine the fieldtype
of the object. But I think this extra complexity (and additional failure cases
to consider) isn't worthwhile to add.
@vtjnash vtjnash requested a review from JeffBezanson April 29, 2019 21:55
@StefanKarpinski
Copy link
Member

What is the benefit of making this lazy?

@vtjnash
Copy link
Member Author

vtjnash commented Apr 30, 2019

It avoids the cost of allocating it until it's actually referenced, so it saves a bit of time and memory.

@vtjnash vtjnash merged commit a6c7c1b into master May 6, 2019
@vtjnash vtjnash deleted the jn/lazy-ftypes branch May 6, 2019 22:53
@JeffBezanson
Copy link
Member

I'm not sure I like the later error throwing this results in. Maybe we should still throw the error when constructing concrete types? After all, we are still paying for instantiating the field types in that case. This has some precedent; for example constructing Complex{Array{T}} where T is allowed but Complex{Array{Int}} is not.

A further concern is compiler performance. This didn't occur to me at first, but say we have X{T}(a, b) in code, where X{T} would have invalid field types. Previously, X{T} is an error so we don't need to look at the constructor. Now, we have to infer into X{T}( ) and the error is thrown from inside it.

@vtjnash
Copy link
Member Author

vtjnash commented May 8, 2019

That does seem like an interesting compromise. On the other hand, throwing an error here could mean that subtyping performance is potentially instead reduced in the case you describe, since we become unable to represent and cache the subtype result. So, instead to the situation you described (where we would run into subtyping error and thus be required to repeat this computation at runtime), it would be able to infer forwards to error it must result in, thus actually potentially having a positive performance implication for the compiler. I'm theorizing that the error might now actually happen at a more expressive place. But I'm also hoping that by merging this, you might find some real-world cases where this makes a difference so we can talk about actual benchmarks and user impact, than just guesses.

After all, we are still paying for instantiating the field types in that case

I think that's not actually precisely true. (Indeed, I had tried to add more comments to this effect to the merge description, but GitHub discarded them when executing the merge.) After this PR, we only pay the cost if the answer does not affect the subtyping answer. But people have constructed cases (See discussion in #25796 and #31064) where we can't give a consistent answer if allocating the fieldtypes for a concrete type might throw (since subtyping defines that throwing an exception here means !subtype, but per those issues, that's logically unsound). It's unclear to me what you would then be able to do with that inconsistency, but I would rather not add a promise that subtyping will take into account this information side channel.

@JeffBezanson
Copy link
Member

since subtyping defines that throwing an exception here means !subtype

Do you mean intersection? I don't see where subtyping does this.

required to repeat this computation at runtime

Wouldn't it be a case where an error is thrown at runtime though? I also imagine that this would be more likely to occur in code paths that don't run.

But, I think the most important thing is to decide when we want the error to happen, for user friendliness, simplicity, etc. The performance implications seem minor enough that we don't have to worry about them primarily.

@vtjnash
Copy link
Member Author

vtjnash commented May 8, 2019

OK, I only saw it was in the subtyping file, not which parts specifically currently call this constructor. We know that this code is currently reachable, because that was filed as an open issue (#25796). I'm expecting that the error will be thrown by the argument that could not be converted to the declared fieldtype. Actually, I'm guessing that's probably unchanged from the present situation, since the requested type doesn't exist to pass as an argument either. But if you encounter a real situation, I'd like to see what the error looks like, instead of these continued hypotheticals. I certainly hate discarding any error message too.

@JeffBezanson
Copy link
Member

We know that this code is currently reachable, because that was filed as an open issue (#25796).

Which code? I might be missing something; I don't see how field types enter into that issue.

I'm mostly just talking about this case:

julia> struct X{T}; x::Complex{T}; end

julia> X{String}
ERROR: TypeError: in Complex, in T, expected T<:Real, got Type{String}

which no longer errors. As you say, the error is now thrown if you try X{String}(...).

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