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 a macro for out-of-heap block header #9564

Merged
merged 1 commit into from
May 19, 2020

Conversation

kayceesrk
Copy link
Contributor

@kayceesrk kayceesrk commented May 14, 2020

Following up on #9534, we need a way to allow the users to create a valid header for out of heap objects while migrating away from using naked pointers. This was raised by @garrigue in the discussion on #9534. This PR adds a macro definition:

#define Caml_out_of_heap_header ((header_t)(3 << 8)) /* matches [Caml_black] */

to caml/mlvalues.h. The GC in naked pointer mode examines the header, and if it is Caml_black, it does not scan the object.

The idea is to use this header for out of heap objects. For example,

#include "caml/mlvalues.h"

struct {
  value h;
  int i;
  double j;
  void* v;
} s = {Caml_out_of_heap_header, 42, 0.0, NULL};

value get_struct_as_value (value v) {
  return (value)&s.i;
}

Multicore OCaml uses a different GC colour scheme, but has a NOT_MARKABLE colour whose bit pattern is the same as Caml_black. When the GC colour scheme is updated, this API can remain the same.

It would be nice to get this change in 4.11 as we can suggest this as the solution for out of heap pointers.

@kayceesrk kayceesrk changed the title Add a header macro for out-of-heap blocks Add a macro for out-of-heap block header May 14, 2020
@stedolan
Copy link
Contributor

This sounds good!

I think the API should be something like Caml_out_of_heap_header(tag, size), though, since it is useful to have off-heap objects that are manipulated by ordinary OCaml code that expects a valid tag and size (e.g. large constants)

@kayceesrk
Copy link
Contributor Author

Thanks for the suggestion. I've made the necesary changes.

@gasche
Copy link
Member

gasche commented May 18, 2020

In your example struct, you have value h; when I would have expected header_t h;, is this just a typo?

@kayceesrk
Copy link
Contributor Author

In your example struct, you have value h; when I would have expected header_t h;, is this just a typo?

Yes. It is. I confirmed that the type header_t is exported by mlvalues.h.

the OCaml heap, now points inside the OCaml heap, and this can crash the
garbage collector. To avoid these problems, it is recommended to wrap the
pointer in an OCaml block with header "Caml_out_of_heap_header" defined in
"caml/mlvalues.h".
Copy link
Member

Choose a reason for hiding this comment

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

With this documentation change, the suggestion to use Abstract_tag or Custom_tag is gone. Is it not the recommended approach anymore (is it only correct in presence of the page table?)? Is Caml_out_of_heap_header a generalization, or complementary?

My non-expert understanding is that Abstract_tag and Custom_tag are no-scan-tags, so the GC will not traverse the block (whatever the liveness property of the block is), and Caml_out_of_heap_header can contain arbitrary tags but are always marked already-traversed for the GC.

Copy link
Member

Choose a reason for hiding this comment

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

Note: I think that the example struct you had in the PR description may be useful in the documentation as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With naked pointers, the only requirement is that the headers are marked black. It doesn't matter what the tag is. The issue with Abstract_tag and Custom_tag with the color being non-black is that the GC will write to the header while marking. This may not be appropriate if the out-of-heap region is marked as a read-only section. This was pointed out by @stedolan.

I don't want to recommend Custom_tag since a custom object is expected to have a block of custom operations as its first field. Not having the correct first field may cause a segfault on marshall and polymorphic comparison. Better to recommend Abstract_tag as the code will get an Invalid_argument exception on marshall and polymorphic comparison.

I'll edit the documentation appropriately now.

Copy link
Member

Choose a reason for hiding this comment

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

So if I understand correctly, we need to use Caml_out_of_heap_header to avoid GC-related traversals, but we also need to pick the right tag for the other runtime operations (comparison/hashing), Abstract_tag being recommended as the default blackbox.

(Is there an approved/recommended way to masquerade pointers as non-pointers by tagging them? We use this technique in a couple places in the runtime, and I suppose it would be useful (and probably already used) for some FFI situations. When would one use this rather than a Black Abstract?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I understand correctly, we need to use Caml_out_of_heap_header to avoid GC-related traversals, but we also need to pick the right tag for the other runtime operations (comparison/hashing), Abstract_tag being recommended as the default blackbox.

Yes. For non-expert users, who may want to just have a pointer to an out-of-heap structure, Abstract_tag is appropriate. Operations such as comparison and hashing will raise an exception. If you do want hashing and comparison to work, then (the expert) users will need to choose the right tag and size and corresponding object layout based on the need.

(Is there an approved/recommended way to masquerade pointers as non-pointers by tagging them? We use this technique in a couple places in the runtime, and I suppose it would be useful (and probably already used) for some FFI situations. When would one use this rather than a Black Abstract?)

We use this in multicore bytecode runtime to tag the code pointers so that they're not followed by the GC (multicore does not use page tables). https://github.com/ocaml-multicore/ocaml-multicore/blob/172fe18bbc7fcdaa1fc286869c28a2c59ecbf27a/runtime/caml/mlvalues.h#L265-L281. This is an example of a situation where tagging rather than having a Black Abstract header makes sense. That said, we should discuss pointer tagging in a separate issue.

@kayceesrk
Copy link
Contributor Author

@gasche I've added more documentation including explaining that the size argument in Caml_out_of_heap_header does not matter if the tag is Abstract_tag. The documentation is correct, but not complete. For example, if the tag were chosen to be Infix_tag, in no-naked-pointer mode, the size does matter. But this is digging too much into the details of the runtime system. Current documentation is correct and can stay the same when switching to naked pointer mode by default in later releases. At that point, we will just make it a strict requirement that the out of heap blocks should have a valid header; currently, it is only recommended. And the same documentation will work for when multicore lands.

Appveyour failure is spurious and should work if restarted again.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Thank you for the clarifications and the example. I apologize for the space&time taken by the documentation discussion, but I think that it is nice to store our shared understanding in the documentation. (cc @xavierleroy who may be interested in this as well.)

Thinking about this more, there are still things that I don't understand. I now have the impression that what the documentation meant by "wrapping" before is: instead of storing a naked pointer as a value, allocate an Abstract or Custom block on the OCaml heap whose payload is this pointer. (It's a shame that the documentation didn't come with a small code example which would have made this clear.) This suggests that the paragraph you modified and the new content are in fact talking about twp different things:

  • If we want to store an arbitrary pointer, it should be the payload of a No_scan_tag on the OCaml heap (and there the color should stay the usual color).
  • If we have control over the memory at the destination and we don't want to allocate a wrapper on the OCaml heap, we can use an outside-heap pointer as a value, provided that it has a correct block structure and the word before the destination is a Caml_out_of_heap_header header. In particular, if the header has the Abstract_tag tag, there are no structural requirements on the payload (not even the size).

In the first case I think that mentioning Custom_tag makes perfect sense (it is natural to expose third-party resources from the C world and provide comparison/hashing operations on them).

(I thought the structure-packing example was maybe a bit advanced/arcane in comparison to the rest of the documentation. Is it a frequent need?)

reallocated as part of the OCaml heap; the pointer, formerly pointing outside
the OCaml heap, now points inside the OCaml heap, and this can crash the
garbage collector. To avoid these problems, it is recommended to wrap the
pointer in an OCaml block with a header consutrcted using
Copy link
Member

Choose a reason for hiding this comment

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

constructed

@kayceesrk
Copy link
Contributor Author

Right. I misunderstood what the original documentation meant. And I can see now that what the original documentation meant was allocating a wrapper on the OCaml heap. Thanks for the clarification. I shall fix the documentation.

Also, there's the third option of tagging the pointer so that GC sees the pointer as an immediate. Of course, the caveat is that the pointer has to be aligned such that the least significant bit is 0. What are your thoughts on documenting that?

@kayceesrk
Copy link
Contributor Author

(I thought the structure-packing example was maybe a bit advanced/arcane in comparison to the rest of the documentation. Is it a frequent need?)

I want to convey that the size argument doesn't matter if the tag is Abstract_tag. It is good to have documentation about this as otherwise, it is unclear what the size should be in case of a structure whose size isn't multiple of word size.

@xavierleroy
Copy link
Contributor

I'm afraid we have a big documentation problem here.

To manipulate pointers to external resources from OCaml, the one recommended approach that everyone should follow is to put the pointer inside an Abstract or Custom block, with a preference for Custom so that finalization can be implementated.

The new macro for out-of-heap block header is a substitute for the old, not recommended "naked pointer" approach. It's a way to have a naked pointer that looks enough like an OCaml value to fool the GC. But like naked pointers it doesn't work for dynamically-allocated resources. So it's only possible use is for statically-allocated data. Legitimate uses are few. There might be a couple in lablgtk. But this is the super-advanced technique that nobody should use, really.

There is something wrong: the super-advanced technique that nobody should use really is documented in much more details and examples than the recommended technique that should be used everywhere. It's like a user's manual for a length of rope: "It can be used for climbing. But now we'll explain in full details, including examples, how you can use it to hang yourself".

@kayceesrk
Copy link
Contributor Author

@xavierleroy I realize that I trampled over sensible documentation. I can revert the documentation changes, and leave the new API undocumented as it is meant for advanced use cases. Is this the way to go?

@gasche
Copy link
Member

gasche commented May 19, 2020

Maybe reverting to the original documentation (hopefully: slightly clarified, to avoid misunderstandings, and why not with a small example of pointer wrapping) is the right thing to do for this PR, so that @kayceesrk can move on to other things.

But @xavierleroy, a meta point: I think that the "documentation problem" is not really that advanced methods are detailed, but that the rest is not really documented. The idea that things that are delicate/advanced need not be documented results, after a couple decade, in a big mess where everyone is copying dirty tricks that they learned from reading the source of foo (with no documentation, of course) and may or may not work, but "foo" sounds like an important project so hopefully the runtime will keep making it work.

We are at a time where the programming model for the OCaml FFI is changing, and I think it is particularly important to try our best to document this, at least to ensure that the language maintainers and contributors agree on the change. This could be in a technical document somewhere, but writing it in "advanced" sections of the manual (1) forces us to be clear and explanatory, and that leads to improved undertanding and (2) has the side-effects that other language users benefit from the solidified knowledge.

I think we should have more documentation, not less.

@xavierleroy
Copy link
Contributor

I knew @gasche would react immediately, so I waited a bit :-)

In the very short term, I agree that the best thing to do is remove the documentation change from this PR, so that it can be merged without further delay and @garrigue can start using the new macros in lablgtk.

In the short term, I would propose to reword the documentation of naked pointers in the manual to say that they should no longer be used because they will stop working very soon, and to better explain the wrapping in a Custom block approach. Note that the "complete example" of section 20.6 uses Custom blocks already.

In the longer term, there might be a book to write on "Secrets of the OCaml runtime system". I still think it should be distinct from the reference manual.

I think we should have more documentation, not less.

I think we should make the documentation more useful, not more confusing. Usefulness is not proportional to quantity.

@kayceesrk
Copy link
Contributor Author

Done.

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

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.

None yet

4 participants