Move layout from Type_abstract to type_declaration

[goldfirere]

This was extracted from #1340, but it's quite separate and makes more sense as its own PR.

This takes the layout of a type out from the Type_abstract node and puts it right in the type_declaration. I think it's a little cleaner this way, but the real motivation for this is to fix performance problems that arose in #1340.

[goldfirere]

Don't review yet. I have some improvements in the works.

[goldfirere]

The "improvements" might be controversial (though I like them), so I pushed them off to #1386. This is ready for review.

[goldfirere]

@ccasin observed that the layout in Type_abstract is very like the layout in Record_unboxed and Variant_unboxed, so I removed those, too, in the commits above.

[ccasin]

I agree this is cleaner - I've left some comments and questions throughout.

However, I'm wondering about this program:

type t_void [@@void]

type ('a : any) t = A of 'a

type s = t_void t [@@immediate]

Obviously, we don't allow the any parameter today, but we plan to. s should check, because 'a t is immediate just if 'a is void.

I don't actually think either the old code or the new code gets this quite right (even if we imagine they allowed the ('a : any)). The new approach is wrong because we just trust the stored layout on the decl of t. The old approach was wrong because layout_bound_of_kind trusted the stored layout in the variant_representation - it didn't consider parameter instantiations.

One can imagine a few approaches to this. We could soup up layout_bound_of_kind to really recompute the thing but with parameters instantiated (probably sharing code with typedecl). We could record dependencies on parameter layouts, either in the variant and record representations or in the overall decl (or both).

Maybe it would be more productive to talk through this example, but for the moment I'm less convinced about dropping layout_bound_of_kind (whether or not we move the layout for abstract types from the kind to the decl).

[goldfirere]

@ccasin:

However, I'm wondering about this program:

...

Maybe it would be more productive to talk through this example, but for the moment I'm less convinced about dropping layout_bound_of_kind (whether or not we move the layout for abstract types from the kind to the decl).

Yes, I've been worried about such programs, too. My very-sketchy plan was to actually make type_layout : layout list -> layout in the glorious future, where the input list comprises the layouts of the type arguments. That's not quite right, because most decl layouts are constant, so we'd ideally want layout Lazy.t list -> layout but more realistically probably some other more inspectable structure that says which argument layouts it really needs. Maybe the lazy one would work? Not sure how expensive making thunks is. I think this general plan would work, and would pay other dividends, too, in that (I think) we'd never have to expand in order to get a more precise layout (but I've gotten that interaction wrong before).

As you say, I think the problem you highlight here is orthogonal to this patch (but definitely a problem).

I don't think we can keep layout_bound_of_kind and also move the layout out from Type_abstract. Essentially, the action of layout_bound_of_kind would be closed over in my layout Lazy.t list -> layout.

[ccasin]

I'm not sure the example is entirely orthogonal.

Here's a different example (which we've already been talking about) that maybe illustrates why I think the problems are related. Consider projecting x from a record like:

type ('a : any) t = { x : 'a; y : int }

To compile the projection, at some point we'll need to get the sort of whatever type 'a has been instantiated with. How to implement this?

My guess at the moment is that the right thing is to enhance record_representation to record the dependencies of each field on the parameters (similarly to what we are discussing for type_decl). But since we definitely have to do something like this for the individual fields, it feels cleanest to solve the problem of the immediacy check in the original example with the same mechanism. That is, we should keep layout_bound_of_kind and have it use this recorded dependency information if needed, rather than duplicating this in both the record_representation and the type decl, which feels finicky and error-prone (can they get out of sync? which one do we check when?).

I think keeping layout_bound_of_kind is compatible with moving the type_abstract layout from the kind to the decl if you also make the decl's layout bound an argument to layout_bound_of_kind. Though maybe the move feels less well motivated then.

[goldfirere]

Consider projecting x from a record like:

type ('a : any) t = { x : 'a; y : int }

To compile the projection, at some point we'll need to get the sort of whatever type 'a has been instantiated with. How to implement this?

So you're worried about e.g. let f (t : string t) = t.x, t.y, right? I've put in a type annotation here because of course we can't proceed without fixing the layout of x. This looks fine to me: we'll have to determine the type of t.x in order to compile the expression t.x. Then the sort of the type of t.x is apparent from the type. Am I missing something?

Maybe more interesting is projecting t.y. The type-checker will just say that t.y is an int and carry on. But the code generator will need to know x's layout in order to get to y. So (in my formulation) we'd probably need some kind of layout Lazy.t list -> layout array that gives the layouts of the components of a record from the layouts of the type arguments to that record. (This might not want to be lazy, because it seems quite likely that all the input layouts will be needed.) Maybe you're saying that we should then compute the overall layout of the record from the output of this other function? Plausibly, but I think it would be less efficient to do so. Almost all records will be values, irrespective of the layouts of arguments. So computing the record layout by first computing the layouts of the components seems like more work than necessary. I suppose we could store that the overall record layout is always value separately.

Bottom line for me: I see your point about keeping things in sync. I think it's valid. But I still don't see how this concern should stop us from merging this patch, mostly because the concerns you're bringing up are just as valid before this patch as afterwards. (Well, not quite: if we always compute a type's layout by computing the layouts of all of its fields -- as is currently the case -- then I see how keeping things in sync is guaranteed. But in the glorious future with any fields, doing so will be even more expensive than it is today.) And figuring out the answers now is unnecessary (though we will have to figure these out soonish).

The thing I care most about in this patch is including the layout in the type_declaration. That means we can just grab it, instead of recomputing it every time. Doing this simplifies the algorithm in type_decl (to my eyes) and is more efficient. We can also keep layout information elsewhere, too. I don't think we should, but I don't care about that too much.

[goldfirere]

OK. I've pushed some updates, including a big new comment in Typedecl. That comment ends describing an infelicity of the current design -- but I claim that the infelicity predates this patch.

[ccasin]

This thread now has a lot of text - let me know if it would be easier to have a discussion, but I've tried to explain myself below. The short version is that I'm fine with putting the layout in the type decl, it's certainly an optimization (but I think a small one even in the scary future of the examples). But eliminating layout_bound_of_kind seems like a mistake because we're going to want it in that scary future.

So you're worried about e.g. let f (t : string t) = t.x, t.y, right? I've put in a type annotation here because of course we can't proceed without fixing the layout of x. This looks fine to me: we'll have to determine the type of t.x in order to compile the expression t.x. Then the sort of the type of t.x is apparent from the type. Am I missing something?

Right, I'm concerned about examples like that. Or, not concerned, but I think the right solution provides motivation to keep layout_bound_of_kind.

I do not think we want to check this by first computing the type and then computing the layout of the type. That sounds expensive (e.g., if the type is an alias, we will have to do expansion). We want a fast path where we just look up the layout for a projection in the record representation, and a fallback path where we do something more expensive (but hopefully still less expensive than a full type -> layout calculation).

That fallback path could compute the layout from the (fully instantiated) type. But we're already discussing that rather than doing that in the case of type_layout, we want some smarter thing that records dependencies on layouts of type parameters. I'm proposing that smarter thing goes in the record and variant representations, and then we have a uniform solution for dealing with both examples (the projection example and the [@@immediate] example) - both compute the information they need from the kind, which knows about dependencies on the layouts of type parameters.

Maybe more interesting is projecting t.y. The type-checker will just say that t.y is an int and carry on. But the code generator will need to know x's layout in order to get to y. So (in my formulation) we'd probably need some kind of layout Lazy.t list -> layout array that gives the layouts of the components of a record from the layouts of the type arguments to that record.

I agree we want something like that. One option is for the record representation, in addition to storing a layout for each field (optionally parameterized by the layouts of each parameter), to also store an offset for each field (optionally parameterized by the layouts of each parameter, as well). I agree some use of laziness here may work well.

Maybe you're saying that we should then compute the overall layout of the record from the output of this other function? Plausibly, but I think it would be less efficient to do so. Almost all records will be values, irrespective of the layouts of arguments. So computing the record layout by first computing the layouts of the components seems like more work than necessary. I suppose we could store that the overall record layout is always value separately.

I think what I'm proposing is mainly a way of doing this computation, when it's necessary, cheaply and using the same system we use to deal with the projection example. I think it would be fine to still stick a layout in the decl, and not stick one on Type_abstract. Today that layout will be completely accurate if we have a record or variant kind, but the observation of the [@@immediate] example is that soon that won't be true and we'll need some strategy for falling back to a more complicated check when necessary.

I agree that when we have to do the fallback in the case of my first example, having the parameter-dependent computation in the decl would be marginally cheaper than having the parameter dependent computation in the record-representation. But one key observation of Stephen's original approach to immediacy is that crawling the kind isn't what's expensive, expansions are what's expensive.

So: the second example shows we're going to need complex record_representations and variant_representations that record dependencies on the layouts of type parameters. Further, the performance difference between the two cases is (a) small and (b) only hit in the uncommon case of non-abstract types with parameter-dependent layouts (if we also store a layout bound in the decl). So let's just have that dependency system in one place (the record/variant representations).

Bottom line for me: I see your point about keeping things in sync. I think it's valid. But I still don't see how this concern should stop us from merging this patch, mostly because the concerns you're bringing up are just as valid before this patch as afterwards. (Well, not quite: if we always compute a type's layout by computing the layouts of all of its fields -- as is currently the case -- then I see how keeping things in sync is guaranteed. But in the glorious future with any fields, doing so will be even more expensive than it is today.) And figuring out the answers now is unnecessary (though we will have to figure these out soonish).

The thing I care most about in this patch is including the layout in the type_declaration. That means we can just grab it, instead of recomputing it every time. Doing this simplifies the algorithm in type_decl (to my eyes) and is more efficient. We can also keep layout information elsewhere, too. I don't think we should, but I don't care about that too much.

I'm concretely proposing that the part of this patch that eliminates layout_bound_of_kind is a mistake. We're going to need complex machinery in record and variant representations to deal with the projection case, and we should plan to reuse it for (the hard cases of) computing declaration layouts.

I think it's fine to move the layout into the decl, but I think ultimately the performance impact here is small. layout_bound_of_kind is very cheap today (or at least was before the histories stuff, and is easily made cheap again as discussed). It can continue to be very cheap even in the scary future, using laziness as you've suggested (or some similar mechanism) - the common case will be that we don't have to consider parameter layouts at all.

That said, caching a layout bound in the type_decl is fine, and is certainly an optimization. I'd suggest we change this PR so that, when doing a layout check, we first check the cached layout and then fall back on layout_bound_of_kind if it fails. At the moment, that fall back case will always fail if it is reached. But this fallback behavior will be needed in the future. Alternatively, leave a comment at the places where you've removed the calls to layout_bound_of_kind mentioning that it will likely come back.

[ccasin]

On reflection, I don't feel super strongly about keeping layout_bound_of_kind, if we're going to put a layout in type declarations. I think what I said above is true, but keeping the dead code because we're going to want it in the future is not the right solution. We can evaluate whether we want to add back something like it when we deal with those complicated cases. Maybe the comments explaining that the type_layout in decls is exact in the case of record and variant kinds should be updated to indicate this may not always be true.

Having thought about these future cases where we're going to need to deal with substitutions, I appreciate Stephen's original design more. Having the layouts in the places where they matter and only there is parsimonious, and has minimal performance impact. In a vacuum, I'd keep the way it's done now, but I don't feel strongly about it and you clearly do, so I think this PR is fine (I'll approve after I look through your responses to my comments).

This PR will make some of the info in record and variant representations dead code, as your subsequent PR shows. But it's clear after having thought through these examples that we can't get rid of that info permanently. And in fact I'm pretty sure we're going to want them sooner than that - even just for unboxed floats (at least, when we allow unboxed floats in structures). So we should put that subsequent PR on hold - we can reevaluate after shipping the unboxed float stuff, in case I'm wrong about wanting this info there.

[goldfirere]

We seem to be converging here. As I said above, I'm not specifically against keeping layout_bound_of_kind, though -- as you say -- it would be dead code for a little while. And so I advocate removing it -- it's not very complicated. My suspicion is that when we get to the point of restoring the functionality in some way, things will look quite different than they do today, and we'll just solve the problem before us. One step at a time!

Part of that suspicion is informed by my suspicion that we can get rid of the need for expansion in layout calculations altogether, with the layout Lazy.t list -> layout approach. But I haven't worked that out and don't need to today. All in good time.

[ccasin]

OK. I think we're agreed. I have some suggestions about your nice new comment, but this can be merged once you're happy with that comment.

[gretay-js]

for files owned by me.

[mshinwell]

Only files I review