-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Unboxed types #606
Unboxed types #606
Conversation
About the let rec issue, another solution would be to make x equal to Val_unit. It actually has the intended semantics. |
A warning or error might be appropriate, but it should not just trigger the optimisation because there is a related optimisation: unboxing the fields and constructor arguments themselves. For example,
could eventually mean that the |
From a quick scan, it is not clear to me whether the issue of the float array hack is handled properly. For example, what happens in the following case:
|
I really whish this could be made the default. Putting aside backward compat, there are no downside at all of this optimization (the let rec issue is inconsequential for most users ..). If I understand correctly, this will break FFI only if the C side construct/destruct values from a datatype with one constructor, is that right ? Does that even happen in practice ? |
It's a bit more complex than just
Anyway, that's more of a discussion for #556 as it's pretty orthogonal to the present optimization. |
Indeed. Your example is not enough to trigger the problem but if I do:
then I get a (flat-allocated) float array where I can store [edit: it does segfault after fixing another bug that was hiding this one] |
Indeed, I only noticed the |
I think what is needed is a check for whether the argument can be either a float or something else. It is hard to define this property precisely. I think a rule allowing only the following three cases would be sufficient, but may be overly conservative:
|
Is there any hope/plan of making this the default in the future? |
I'd like to get some feedback on this. The incompatibilities don't seem to be really problematic, so right now I'm on the fence. |
My view is that, except for the c binding issue, it's a pretty clear win. One could imagine a reasonable transition story: start with a flag to turn it on by default, and over time migrate to making it opt out instead of opt in, and then finally remove the old behavior. If that was available, I believe we'd use that and maybe never bother with the annotation. |
I'm personally in favor of making this the default but I've a general tendency to be rather liberal in terms of breaking backward compatibility when useful. Do we have a way to asses the impact of making this the default? I guess there aren't so many sum/record types with a single constructor/field, so perhaps a pass on public OPAM packages, excluding pure packages without C bindings, could give a good indication. I'd like to point out also that the impact could be in theory larger than C bindings. For instance we have code at LexiFi that processes our runtime type representations and does some (Obj.)magic with the concrete representation of values. The same could happen with code-generators. Of course people using this are on their own (even though they make the same assumptions as in C bindings). Another potential issue is with alternative backends such as js_of_ocaml or Bucklescript (some Javascript code assuming a specific representation of OCaml values). About the feature itself: this could come later, but I'd also love to be able to unbox specific constructors. Typically it should be possible to unbox at most one (in a given sum type) constructor taking a "string" argument. One could also support "unboxing" a constructor whose argument is itself a sum type (allocating tags properly to avoid clashes). |
I was thinking of simply making it the default and providing an annotation to turn it off on a specific type. |
I'd be a little hesitant about making it the default whilst the float array hack still exists, because then some existing type definitions will have to become errors until annotated with |
This would need major changes to the compilation of pattern-matching. Indeed it will come later, if ever. |
I would like to be able to unbox float, int32, int64, and nativeint within records and algebraic datatypes. But that requires changes to the runtime (specifically the GC). |
Not just the GC, but also generic comparison and generic serialization. |
My solution is to have all pointers contiguous at the start of the object.
|
One needs somehow to mark which prefix of the block must be scanned (i.e. contain normal values). This could be done in several ways:
Generic operations would need to be adapted as well. Without keeping more layout information about these mixed blocks, it will be impossible to preserve the exact same behavior (e.g. inlined floats cannot simply be compared bit-wise), but this is probably fine as long as it is documented (at least if unboxing fields is explicit). |
My thought is to have the first word in such a "mixed block" have the My preference is for unboxing of fields to be the default at some points. Yes, this will break C stubs, but I also think that OCaml should move away from C stubs towards an FFI integrated with the compiler, in which OCaml – not C – is responsible for marshaling of data, either inline within generated code (for ocamlopt) or by compiler-generated C code (for ocamlc). As for structural comparison, hashing, etc, I think that a bitwise comparison would be enough. I actually think that structural comparison/hashing of mutable objects is a misfeature, and that mutable objects would be better compared/hashed by object identity. But this is the wrong discussion for that. In any case, the only alternative that I can think of is to either (1) do a type lookup for each and every type (using even more metadata) or (2) do mandatory specialization of comparison and hashing (but what about polymorphic recursion? |
Unfortunately, that breaks compaction because of infix closure pointers. |
My proposal would still treat closures specially (or not, with GPR #203). The first word (which points to the compiled code for the closure) would always be treated as a non-pointer (since compiled code never moves). |
The consensus at the latest developer meeting (2016-06-16) was that this should not be the default at first, until users of the FFI (especially the likes of ctypes and camlidl) have adapted. Also, it would be nice to design a tool that can help with the transition. |
Would it make sense to add a flag that flips the default? That way, we could try it out inside of our walls, and perhaps learn more about the FFI issues. It would be nice, at least within our walled garden, to get the performance benefits without needing to litter our code with annotations. I suppose an alternate approach would be for us to write a PPX that automatically adds the annotation to every single-entry variant. |
Are you thinking of a configuration flag or a compiler flag? One problem with making it the default: when checking GATDs for unboxability, the conditions are rather complex, so I'd rather make |
I was thinking of a compiler flag, since it would allow you to change the behavior in a library by library way. For us, we'd likely leave the default behavior as is for externally developed libraries. @lpw25 @diml : what do you guys think about the wisdom of having a compiler flag to flip the default, versus a PPX to determine the behavior? If we do the PPX, it will be easy for us to work out in practice what seems most convenient, and then maybe we can use that to inform what the API should look like in the compiler longer term. |
b69bc5e
to
1c52075
Compare
@alainfrisch I think it's cleaner like this. Could you review again? Also, we need to agree on the name. |
match Env.find_type p env with | ||
| {type_unboxed = {unboxed = true; _}; _} -> | ||
Misc.Stdlib.Option.value_default (fun x -> x.desc) ~default:sty | ||
(Typedecl.get_unboxed_type_representation env ty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this logic be moved to Typeopt.scrape
in order to benefit to other functions that depend on it (e.g. is_base_type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Having is_base_type
covered is useful for Translcore.specialize_comparison
, for instance.)
1c52075
to
2a5c1c6
Compare
@alainfrisch I've implemented both of your suggestions. Can we merge now? Do you still want to discuss the name? |
I'm not fully confident with the restrictions related to unboxed float arrays, but I don't think that more code review will address that on my side. So, for me: "yes".
Nope! |
3c6eddb
to
9cc74e7
Compare
9cc74e7
to
d5a6e50
Compare
@@ -150,6 +151,7 @@ let number = function | |||
| No_cmx_file _ -> 58 | |||
| Assignment_to_non_mutable_value -> 59 | |||
| Unused_module _ -> 60 | |||
| Unboxable_type_in_prim_decl _ -> 61 | |||
;; | |||
|
|||
let last_warning_number = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@damiendoligez This should be bumped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I bumped it in 7fee1ea .
I have just run into an issue with the OCaml 4.04 beta2 using this feature. Consider the following code: type ('a, 'kind) tree =
| Root : { mutable value : 'a; mutable rank : int } -> ('a, [ `root ]) tree
| Inner : { mutable parent : 'a node } -> ('a, [ `inner ]) tree
and 'a node = Node : ('a, _) tree -> 'a node [@@ocaml.unboxed]
type 'a t = ('a, [ `inner ]) tree The above will fail with:
Here is a kludgy workaround: type ('a, 'kind, 'parent) tree =
| Root : { mutable value : 'a; mutable rank : int } -> ('a, [ `root ], 'parent) tree
| Inner : { mutable parent : 'parent } -> ('a, [ `inner ], 'parent) tree
type 'a node = Node : ('a, _, 'a node) tree -> 'a node [@@ocaml.unboxed]
type 'a t = ('a, [ `inner ], 'a node) tree I suspect that the type |
Thanks for the catch, I submitted a bug report to make sure we track this properly: PR#7364. |
Unboxed types
Co-authored-by: tmattio <tmattio@users.noreply.github.com>
This PR introduces an annotation and an optimization for concrete types that have only one constructor with one argument, and immutable records with only one field. In both cases, the default representation is a memory block with header and one field. This memory block represents no useful information and we can get rid of it.
Example:
In this case, the pattern-matching does not even read the header of the block that represents
A s
because it contains no useful information. With this patch, if we add a[@@unboxed]
annotation on the type definition, the compiler will suppress the indirection block and represent the value directly as the string:This is useful (for example):
Some questions
unboxed
is already used for a different (but related) purpose. Is it a good idea to reuse it?[@unboxed]
annotation on the constructor or on the record field, it is ignored. Should it trigger a warning or error, or even just trigger the optimization anyway? (In fact, the same problem already exists with[@immediate]
).Future work
Some more work is needed to make it work nicely with the float array and float record optimizations: currently, if you write:
then a value of type
t
is represented as a float, but a value of typer
is not optimized into a float array, unlike types
.A related problem is the optimization of array access for
t array
and the interference with the[@@immediate]
annotation (type t = A of int [@@unboxed] [@@immediate]
currently fails).[update: all the above are now implemented]
Compatibility
If activated by default, this optimization will break the FFI because it changes the representation of values. It will also break the compatibility with old marshalled values.
A more subtle incompatibility is with
let rec
:This must be rejected because it is (compiled as) the same as
let rec x = x
. The current compiler rejects it without any need for a patch but #556 will be made slightly more complex by this PR.