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

Remove Meta.static_{alloc, free}. #1723

Merged
merged 5 commits into from May 28, 2018

Conversation

Projects
None yet
5 participants
@stedolan
Copy link
Contributor

commented Apr 13, 2018

The Meta.static_alloc and Meta.static_free functions return naked pointers to blocks of memory allocated with caml_stat_alloc. This is awkward for both multicore and @mshinwell's no-naked-pointers mode.

Currently, these functions are used only by the bytecode toplevel and Dynlink, to represent bytecode to be loaded (which is why no-naked-pointers doesn't trip over them, as that mode is currently native only). These modules type the naked pointers as bytes, and manipulate them using various unsafe Bytes functions. (Ironically, unsafe functions are used since the safe ones could segfault, since bounds checks don't work on such raw memory blocks).

This patch changes the toplevel and Dynlink to use Longstring.t to represent bytecode, instead of naked pointers typed as bytes, which affects a number of points:

  • Meta.reify_bytecode now takes a bytes array (i.e. Longstring.t) instead of a naked-pointer-typed-as-bytes. (The type is written as bytes array rather than Longstring.t since the C implementation now assumes the layout of Longstring.t, and this way we get a type error should someone change the representation of Longstring.t).

  • Symtable now only has one version of patch_object, rather than ls_patch_object and patch_object depending on whether the bytecode is a Longstring.t or a naked-pointer-typed-as-bytes.

  • Longstring.unsafe_blit_to_bytes is no longer used, and removed.

  • Longstring.input_bytes_into is added. It takes a Longstring.t instead of returning a new one like input_bytes does, so that it can be used to read part of a Longstring.t from a channel.

  • Meta.add_debug_info and Meta.remove_debug_info were removed. They were only called when creating and releasing bytecode, so they've been integrated into Meta.reify_bytecode and Meta.release_bytecode

  • Meta.release_bytecode now takes a Meta.bytecode (which is an Abstract_tag block containing a pointer and a length) rather than a naked pointer and length, making it safe for GCs that don't have a page table.

I'm aware of one remaining issue before this is mergeable. Dynlink currently calls caml_register_code_fragment, to attach a digest to the new bytecode fragment. However, since 5302d10, Meta.reify_bytecode also registers a code fragment, without a digest. I think this means that a Dynlink'd bytecode fragment is registered twice, both with and without a digest. I'm not familiar with this code, and I don't know whether this is a problem or whether it's intentional (@gasche ? @alainfrisch ?). Would adding an optional digest parameter to Meta.reify_bytecode be the right thing to do?
Hopefully adding an optional digest parameter to Meta.reify_bytecode is the right thing to do.

Remove Meta.static_{alloc, free}.
The bytecode runtime now represents code to be loaded as LongString.t,
rather than as a naked pointer to a bytecode block.

(This commit breaks Dynlink of bytecode, due to an issue about digests)
@gasche
Copy link
Member

left a comment

Unfortunately I know very little of the dynlink/toplevel machinery (my name shows up as I helped review 304c9c9, but I wasn't an expert then and forgot most of the details), so I cannot comment on the Meta-level changes.

I like the change to the LongString API, although I think that a LongString.blit_string function could be profitably added.

Also, wouldn't it be time to rename the module into LongBytes? (This might seem orthogonal, but this PR is suggesting to give this module/type some more visibility in the API.)

LongString.set code (compunit.cu_codesize + 4) '\001';
LongString.set code (compunit.cu_codesize + 5) '\000';
LongString.set code (compunit.cu_codesize + 6) '\000';
LongString.set code (compunit.cu_codesize + 7) '\000';

This comment has been minimized.

Copy link
@gasche

gasche Apr 13, 2018

Member

This is a bit heavy-handed, wouldn't a LongString.blit_string : string -> int -> t -> int -> int make sense?

This comment has been minimized.

Copy link
@stedolan

stedolan Apr 13, 2018

Author Contributor

Good idea, changed.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

I agree about renaming LongString to LongBytes. I'll do that as a separate PR, to keep the diff here short.

@stedolan stedolan force-pushed the stedolan:remove-meta-static branch from b77190e to 7f76b23 Apr 13, 2018

let blit_string src srcoff dst dstoff len =
for i = 0 to len - 1 do
set dst (dstoff + i) (String.get src (srcoff + i))
done

This comment has been minimized.

Copy link
@gasche

gasche Apr 13, 2018

Member

I find this implementation a bit disappointing, as my mental cost model for blitting strings into strings is O(1) rather than O(length), so I would expect blit_string to be O(chunk numbers) rather than O(length), but this is not worse than the existing blit, and hopefully no one uses LongString blits for performance-critical purposes for now. So the code could stay as it is -- mildly disappointing.

This comment has been minimized.

Copy link
@stedolan

stedolan Apr 13, 2018

Author Contributor

I promise to write a faster version when there's a use with len > 7 :)

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

I don't have a 32-bit machine handy, so the overflow logic in LongString wasn't getting tested, since all LongString.ts fit in one bytes chunk on a 64-bit machine. To stress this a bit, I edited the LongString module locally, replacing Sys.max_string_length with 37 (causing lots and lots of chunked LongString.ts), and the testsuite still passes.

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

I suspect that make bootstrap is a better test in this instance than make tests, but that sounds good anyway.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

I suspect that make bootstrap is a better test in this instance than make tests, but that sounds good anyway.

make bootstrap passes also.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

Now that bigarrays are part of the stdlib, we could profitably use a bigarray of bytes here instead of this Longstring stuff.

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@xavierleroy, that sounds to me like a bit of work, might have more-interesting-than-expected performance implications (I would assume that the toplevel creates a lot of small code regions), and is sort of orthogonal to the desired properties of the PR (which is to remove the weird use of pointers). Maybe this could be left for a latter PR?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2018

Using bigarrays avoids the "Convert from LongString.t to contiguous buffer" code in meta.c and could lead to more simplifications elsewhere.

It is popular lately to use small, short-lived bigarrays of bytes (see the Cstruct library), without catastrophic performance, apparently, so I'm not sure what performance implications @gasche fears.

@yminsky

This comment has been minimized.

Copy link

commented Apr 16, 2018

I'm not sure what the cstruct experience has been, but on the Jane Street side, we've seen serious problems with the GC heuristics around custom blocks causing very bad GC performance when churning through many bigarrays of bytes. I believe @damiendoligez is looking at this exact question and how to improve matters, but I think there are serious performance issues lurking in this area.

@braibant also I think has some relevant experience here.

@hannesm hannesm referenced this pull request Apr 16, 2018

Open

new buffer library #881

@gasche

gasche approved these changes Apr 24, 2018

Copy link
Member

left a comment

After thinking more about this and doing a second review pass, I have decided that I like the change enough (and feel the C-runtime part of the change not-too-scary enough) to approve it.

I plan to leave at least a week for other people to comment -- @alainfrisch or @xavierleroy may have reasons to complain.

s_len = caml_string_length(s);
memcpy((char*)prog + off, Bytes_val(s), s_len);
off += s_len;
}

This comment has been minimized.

Copy link
@gasche

gasche Apr 24, 2018

Member

Could you move this "convert from LongString.t to contiguous buffer" function to an auxiliary function, maybe in byterun/str.c? It would make the code easier to read, avoid mixing abstraction level, and it might be useful for someone else.

This comment has been minimized.

Copy link
@stedolan

stedolan Apr 30, 2018

Author Contributor

Done, but I left the function in meta.c. The LongString.t type seems to be quite specific to the bytecode implementation, and unlikely to be of much use to other code which would probably use bigstrings instead. (Eventually, it should probably be replaced with bigstrings here too, pending perf worries).

cf->digest_computed = 1;
} else {
cf->digest_computed = 0;
}

This comment has been minimized.

Copy link
@gasche

gasche Apr 24, 2018

Member

I suppose this is handling a string option parameter? It would be nice if maybe the function could have a comment with the expected OCaml type of the value arguments passed.

This comment has been minimized.

Copy link
@stedolan

stedolan Apr 30, 2018

Author Contributor

Now with match syntax!

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

About the GC heuristics and bigarrays, my hope is that #1738 will improve the situation.

@stedolan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

I have no idea what the Travis failure is about.

@stedolan stedolan closed this May 2, 2018

@stedolan stedolan reopened this May 2, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 28, 2018

ping @stedolan

@gasche gasche merged commit 15c8948 into ocaml:trunk May 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented May 28, 2018

It looks like the CI failure that @stedolan observed was indeed transient, as the new run that he started 26 days ago passed correctly. I went ahead and merged. Thanks for the ping! (And for the work.)

gasche added a commit that referenced this pull request May 28, 2018

@stedolan stedolan deleted the stedolan:remove-meta-static branch Aug 13, 2018

@hhugo hhugo referenced this pull request Mar 30, 2019

Merged

Add support for OCaml 4.08 #761

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.