@stedolan, have you benchmarked the cost of TLS variable access? The multicore branch makes heavy use of repeated TLS access which can have non-negligible costs especially for dynamic linking (see here), and I wonder if it might be worthwhile changing the API even further to have a context TLS pointer as the first argument of C functions instead.

I think that LTS access is a discussion more relevant to the multicore runtime than to the multicore API, which is what is most appropriate to discuss here. The proposed changes do not introduced any difference in the runtime that would be performance-sensitive, or at least nothing as radical as changing where globals are stored. (We may wonder whether moving from macros to functions in some case affects performance, and that question is fully relevant to this PR, but I would expect the answer to be that it does not matter under -O2.)

I think that LTS access is a discussion more relevant to the multicore runtime than to the multicore API, which is what is most appropriate to discuss here. The proposed changes do not introduced any difference in the runtime that would be performance-sensitive, or at least nothing as radical as changing where globals are stored. (We may wonder whether moving from macros to functions in some case affects performance, and that question is fully relevant to this PR, but I would expect the answer to be that it does not matter under -O2.)

Here's where it pertains to the API: if Thread-Local Storage access is slow - and I'm not sure it is, though it certainly is on OSX where TLS doesn't exist for dynamic linking, and I've found articles claiming that it's slow in general - then it might make sense to pass a pointer to the context of the thread for functions that use thread-local stuff (basically all GC functionality), much as object method calls do, and for user C code to keep that thread context around.

This discussion is probably better started from the multicore runtime branch, but I've had little luck getting any feedback there, and since I saw the API changes proposal, I thought I'd bring it up, and anyone with more empirical knowledge than me about TLS performance can weigh in.

@bluddy It requires a function call per access from a function in a shared library that uses TLS. So it would be much cheaper for user C code to keep the context around. This could be avoided if dynamic linkers patched each library with the address of the TLS, but they don't.

However, not all C functions need this context. Only the ones that interact with the runtime do. There are many cases where one just wants to call a library function with. Having to write stubs is just annoying. Yes, it can be automated (and ocaml-ctypes does so), but it is still an annoyance and a small performance hit.

I propose that we make a Foreign Function Interface (that doesn't require the user to write stubs) an official part of OCaml – it, by its very nature, is 100% immune to changes like this. That will require that #724 or equivalent be merged. This FFI could be based on ctypes (not a bad choice in my opinion!), but will hopefully benefit from additional optimizations in the compiler, such as avoiding the stub for any function that doesn't take a struct by value.

@bluddy @DemiMarie Thanks for the thoughts on TLS. The multicore runtime is already actively moving towards a scheme where we do not depend on thread-local variables. Instead, we use a domain-local storage located at the end of minor heap area. This domain-local storage is obtained by suitably bit twiddling the allocation pointer (which is available in a register). Many runtime functions now explicitly accept the domain state as an argument. Eventually all uses of thread-local variables will be removed from the multicore runtime. For continuing the discussion, I've created an issue in multicore: ocaml-multicore/ocaml-multicore#84. Please continue the discussion there.

Let us keep the discussion in this thread focussed on the C API change.

@stedolan, have you benchmarked the cost of TLS variable access? The multicore branch makes heavy use of repeated TLS access which can have non-negligible costs especially for dynamic linking (see here), and I wonder if it might be worthwhile changing the API even further to have a context TLS pointer as the first argument of C functions instead.

On Linux, TLS accesses are about as fast as plain globals in executables and statically linked libraries (PIC or non-PIC code), but about 2x slower than plain globals in shared libraries. Note that most OCaml programs statically link against the OCaml runtime.

On OS X, I believe pthread_getspecific is implemented the same way as Linux's executable TLS, so should be just as fast (On Linux, pthread_getspecific is slightly slower than TLS).

Incidentally, this won't affect the speed of OCaml code: the multicore runtime does not use platform TLS to access domain state while running OCaml code. This is only relevant for C bindings (even then, I can't imagine many C bindings spending a significant fraction of their time looking up globals in the runtime).

It might be better if OCaml had always passed around a context pointer, but there's no compelling performance case to add one now, and it would break a lot of code.

Benchmarks here (may require some makefile hacking to run on OS X)

What about Windows?

In principle I support the change, and I think that we should get it in a released version as soon as possible, to give as much time as possible for possible to update their code (and get as much backward-compatibility as possible when they do). I looked at the patch, but I am not qualified to review it fully, so I'd rather leave that to @mshinwell which has been doing an excellent job at it so far. The point on reading class and oid fields would deserve elaboration, I think.

Sorry it took me so long to update this! Now rebased to current trunk.

I think two things need doing to this:

1. Add a Changes entry
2. Do something with Int_val if there is consensus (I don't see any reason not to do that as part of the same change).

That might be everything?

@damiendoligez Are you happy with this in its present form? It would be nice to get this merged soon.

@stedolan Can you add the Changes entry and do the Int_val change? After that we're good to go.

I've added a Changes entry, but ran into a problem with the Int_val change. The systhreads library failed to build because of this line:

  fprintf(stderr, "Thread %d killed on uncaught exception %s\n",
          Int_val(Ident(curr_thread->descr)), msg);

(-Wformat correctly complained that %d doesn't match a long).

So, a change would have to be guarded by CAML_API_VERSION >= 406.

After a brief discussion with @mshinwell, we noticed another problem. This piece of code is wrong:

long x = Long_val(v)

because on Win64, long is only 32 bits wide yet values are 64 bits. Perhaps the right thing would be to remove both Int_val and Long_val, and replace them with Intnat_val?

(This is getting trickier than I'd thought, and might be best discussed in its own pull request).

@damiendoligez Are you ok if the Int_val changes (etc) are put into another pull request, so we can merge this one now?

Please do. The Int_val thread is incidental to this PR.

@damiendoligez Please approve your review, or we will have to "dismiss" it :)

@mshinwell: the description says "This patch shouldn't be merged until I've updated the "Interfacing with C" section of the manual", but it doesn't appear that the manual changes are included yet.

@yallop Yeah, still waiting for that to be written...

Sorry about the delay, I've been travelling essentially non-stop for the last week, and haven't gotten a chance to write up the docs. It's not forgotten about!

@stedolan Gentle ping on this one. Let's make sure this will be in 4.07!

Ping again: @stedolan, now is the time to write some documentation :-)

I believe the multicore devs are currently doing some experiments relating to GC strategies which may affect the need for some of the changes in this pull request.

Over at a Discuss thread, @rwmjones made the sensible suggestion that the part of this patch that defines CAML_API_VERSION could be made non-invasive and submitted, independently, for quick inclusion. (I can't comment on whether we would have this in 4.07, as we are very late in the release process, but it sounds a reasonable thing to try.)

Would someone be motivated propose a separate PR, for example with just the levels 100 and 400?

I'll see what I can do for a CAML_API_VERSION patch in the next few days. (Also, it wasn't @rwmjones at the Discuss thread. That was me.)

Will multicore change the circumstances under which external functions can be marked noalloc?

Will multicore change the circumstances under which external functions can be marked noalloc?

Generally, no, the same rules apply.

However, the API in this PR replaces Field (which is safe to use in noalloc code) with caml_read_field (which is not), so in the process of adapting code to this API, some functions might cease to be noalloc.

(The annoyance of Field vs caml_read_field and the tantalising possibility of alternative implementations of the multicore minor GC that leave Field alone are what has kept this PR stalled for so long)

An update: with the new "parallel minor GC" that we developed as an alternative collector, we no longer require a read barrier or C API changes. More details of the design are in this draft paper. I'll close this PR until such time as we resume work on a future pauseless collector design which will require a read barrier.