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

Move C global variables to a dedicated structure #8713

Merged
merged 54 commits into from Aug 27, 2019

Conversation

@kayceesrk
Copy link
Contributor

kayceesrk commented Jun 4, 2019

This PR moves C global variables in the runtime to a dedicated structure, a "domain state" table. This PR is a pre-requisite for the multicore runtime, where each domain (a parallel thread of execution) will need to maintain its own table of domain local variables. This PR does not introduce any API changes.

In order to allow fast access to the domain state, we steal the exception pointer register (r14 on amd64) and make it point to the domain state table. The exception pointer is now a field in the domain state table. Convenience macros are introduced to access the domain state fields in the runtime and in the backend of the compiler. The PR is syntactically fairly invasive. But the semantics changes are fewer.

Status

Currently, only exception pointer, young_pointer, and young_limits are in the domain state. The only tested and working architecture is linux on amd64. The build is expected to fail on every other configuration.

  • Introduce domain state and steal exception pointer register
  • Move young_ptr and young_limit to domain state
  • Move multicore relevant native runtime C globals to the domain state
  • Move multicore relevant bytecode runtime C global to the domain state
  • Support Linux x86-64
  • Support macOS x86-64
  • Support Windows cygwin x86/x86-64
  • Support Windows mingw x86-64
  • Support FreeBSD x86-64
  • Support Linux x86
  • Support Windows x86
  • Support Linux arm64
  • Support Linux arm32
  • Support Linux Power64
  • Support Linux IBM Z
  • Support Windows min x86-64
  • Support Windows Native (Microsoft Visual C/C++) x86-64

Changes

Exceptions

On amd64, exception pointers are cached in the r14 register. In this code snippet:

exception Foo

let foo () =
  try ()
  with Foo -> ()

let bar () = raise Foo

the diff in the push trap and pop trap compilation is:

 .L103:
-       .cfi_adjust_cfa_offset 16
-       subq    $16, %rsp
-       movq    %r14, (%rsp)
-       leaq    .L102(%rip), %r14
-       movq    %r14, 8(%rsp)
-       movq    %rsp, %r14
+       leaq    .L102(%rip), %r11
+       .cfi_adjust_cfa_offset 8
+       pushq   %r11
+       .cfi_adjust_cfa_offset 8
+       pushq   8(%r14)
+       movq    %rsp, 8(%r14)
        movq    $1, %rax
-       popq    %r14
+       popq    8(%r14)
        .cfi_adjust_cfa_offset -8
        addq    $8, %rsp
        .cfi_adjust_cfa_offset -8

The exception pointer is stored at 8(%r14). The diff in the raise code is as expected:

 .L104:                                                    
        movq    camlHello@GOTPCREL(%rip), %rax             
        movq    (%rax), %rax                               
-       movq    %r14, %rsp                                 
-       popq    %r14                                       
+       movq    8(%r14), %rsp                              
+       popq    8(%r14)                                    
        popq    %r11                                       
        jmp     *%r11                                      
        .cfi_endproc

Allocations

Inlined allocations sequences are shorter since the young_limit is now in the domain state. The diff in the allocation sequence in the code:

let baz x = (x,x)

is

        movq    %rax, %rbx
 .L101:
        subq    $24, %r15
-       movq    caml_young_limit@GOTPCREL(%rip), %rax
-       cmpq    (%rax), %r15
+       cmpq    (%r14), %r15
        jb      .L102
        leaq    8(%r15), %rax
        movq    $2048, -8(%rax)

young_limit is stored at 0(%r14).

FFI

There is no need to cache and flush the exception pointer when calling C functions. The fact that the exception pointer is not available as a global variable means that functions in amd64.S such as caml_start_program, callbacks, functions for raising exceptions will need the domain state as an extra argument. This change is internal only and the C API remains the same.

Performance

The goal is to retain the performance with respect to trunk. Preliminary performance numbers are available for micro and macro benchmarks. There are a few outliers (in either direction) which I will investigate. And a few, which look like spurious runs (sequence_cps in the macrobenchmark).

Reviewing

It is instructive to start the review at domain_state.tbl which introduces the domain state table. The OCaml and C macros for accessing domain state are generated in utils/domainstate.ml* and runtime/caml/domain_state.h. The interesting changes are in the files asmcomp/amd64/emit.mlp and runtime/amd64.S.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Jun 4, 2019

  • With this change, how far would be we from being able to run multiple independant OCaml runtimes in different threads of the same process? (Exposed e.g. as a way to "restart" the main OCaml entry point in a different thread.)

  • In addition to globals that belong to the runtime system itself, C bindings can also use globals, e.g. to cache "named OCaml values/exceptions". Do you plan to expose an API to allow external C code to store custom data in the "domain state"?

@c-cube

This comment has been minimized.

Copy link
Contributor

c-cube commented Jun 4, 2019

@alainfrisch that's a very interesting idea; would it even be possible to push it further and have both native and bytecode runtimes in the same process, on different domains?

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 5, 2019

With this change, how far would be we from being able to run multiple independant OCaml runtimes in different threads of the same process? (Exposed e.g. as a way to "restart" the main OCaml entry point in a different thread.)

IIUC in this runtime, each thread's heap (minor and major) is completely independent of the other threads? Such a design is not part of the multicore OCaml plan. But I believe it would take a little more work to have such a feature. Specifically, handling signals (pthreads share signal dispositions), fork (threads and fork don't mix well), global roots (multicore has a single global root list, this runtime will have a global root list per domain), etc. Some of these issues will need to be handled in the multicore runtime as well.

In addition to globals that belong to the runtime system itself, C bindings can also use globals, e.g. to cache "named OCaml values/exceptions". Do you plan to expose an API to allow external C code to store custom data in the "domain state"?

Multicore support backwards compatible caml_name_value API (See ocaml-multicore/ocaml-multicore@8d7bc72). There are no plans to extend this API.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 5, 2019

@c-cube I haven't thought through this problem enough. But one problem I see in this approach is build tools; it is unclear now whether bytecode or native versions of libraries must be linked. Do you have a use-case in mind for mixing bytecode and native runtimes?

@c-cube

This comment has been minimized.

Copy link
Contributor

c-cube commented Jun 5, 2019

My use case is quite niche, but it's about having both a bytecode program that embeds OCaml's interpreter (and extends it) on the one hand (so it must be bytecode), and efficient native code on the other hand. Currently the closest I can do is multi-process and message passing, and domains could make it a bit simpler. I don't think it's particularly useful outside of use cases mixing heavy computations and embedding a toplevel.

@bluddy

This comment has been minimized.

Copy link

bluddy commented Jun 5, 2019

One example is an editor written in ocaml that also uses interpreted ocaml for configuration etc.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 5, 2019

Thanks @c-cube and @bluddy. Those are indeed interesting use cases. I'll keep this in the back of my mind while hacking on the runtime system changes for multicore. But I am afraid this feature is not something that I have the time to look at right now.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 5, 2019

@ctk21 kindly analyzed the performance difference in sequence_cps and the difference we see is consistent with the variance that we had observed earlier due to code layout changes. The timeline shows that the current performance difference between trunk and this feature branch is within the historical noise.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Jun 5, 2019

IIUC in this runtime, each thread's heap (minor and major) is completely independent of the other threads?

Yes, absolutely.

Such a design is not part of the multicore OCaml plan. But I believe it would take a little more work to have such a feature. Specifically, handling signals (pthreads share signal dispositions), fork (threads and fork don't mix well), global roots (multicore has a single global root list, this runtime will have a global root list per domain), etc. Some of these issues will need to be handled in the multicore runtime as well.

Wouldn't global roots be stored in the domain state as well, as part of this PR?

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 5, 2019

Wouldn't global roots be stored in the domain state as well, as part of this PR?

Not the global roots. The intention is to only move the domain local C global variables to the domain state. Since the global roots are shared between the domains, the global roots structure remains a C global variable in multicore: https://github.com/ocaml-multicore/ocaml-multicore/blob/master/byterun/globroots.c#L47-L48. I intend to leave the global roots as a global variable.

However, if we wanted a thread-local runtime at some point in the future, then we can move the global roots as a field in the domain state, with the field pointing to the same global roots list in the case of shared heap multicore and different roots list for thread-local rutime. This should be straight forward.

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Jun 5, 2019

One use case I would have for such a completely separated runtime would be to allow writing clean C interfaces for OCaml libraries without hidden global variables. Of course that would require also some changes to the API.

@jordwalke

This comment has been minimized.

Copy link

jordwalke commented Jun 7, 2019

I understand this PR alone doesn't fully implement support for what @c-cube asked about (running multiple OCaml runtimes, each having their own thread in one process). There are many great use cases for this. For example, the Oni editor written with native OCaml/Reason that has a high priority main thread to do UI drawing, but would like to support extensions/plugins which would be extended and mostly isolated from the core editor UI thread.

The other use cases I have is GUI applications that have one thread doing the interactions and animations, and another thread that handles lower priority data processing that shouldn't block the app. They often don't need to share much memory - and it is suitable to create C bindings that lock some serializable resource for communication. Currently we can use OCaml threads, but then there are long periods of time that cores go unutilized.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 9, 2019

All of the domain local C global variables have been moved to the domain state. For other C global variables, I have documented below why they have not been moved. This might be useful for someone implementing the thread-local runtime proposed here. The next step is to implement the change in the different backends.

C globals not moved to domain state

Hooks are installed at the process level. For multicore, we will change the signatures of these functions to take Caml_state as an extra parameter. Using this, the hooks can identify which domains it was invoked for.

  • caml_natdynlink_hook
  • caml_termination_hook
  • caml_scan_roots_hook
  • caml_stack_usage_hook
  • caml_finalise_begin_hook
  • caml_finalise_end_hook
  • caml_major_slice_begin_hook
  • caml_major_slice_end_hook
  • caml_minor_gc_begin_hook
  • caml_minor_gc_end_hook
  • caml_major_gc_hook
  • caml_enter_blocking_section_hook
  • caml_leave_blocking_section_hook
  • caml_async_action_hook
  • caml_sigmask_hook
  • caml_try_leave_blocking_section_hook

Global roots are shared between the domains.

  • caml_global_roots
  • caml_global_roots_old
  • caml_global_roots_young
  • caml_globals_inited
  • caml_globals_scanned
  • caml_dyn_globals
  • caml_incremental_roots_count

Channel list + hooks are for the entire process.

  • caml_all_opened_channels
  • caml_channel_mutex_free
  • caml_channel_mutex_lock
  • caml_channel_mutex_unlock
  • caml_channel_mutex_unlock_exn
  • channel_operations

AFL configuration variables are for the entire process. The details of afl operations under multicore needs to be worked out.

  • caml_abort_on_uncaught_exn
  • afl_area_initial
  • afl_initialised
  • caml_afl_area_ptr
  • caml_afl_prev_loc

Free list. Multicore uses a diffent allocator. Hence, these variables will not be moved.

  • beyond
  • flp
  • flp_size
  • caml_fl_wsz_at_phase_change
  • caml_fl_cur_wsz
  • caml_fl_merge
  • fl_prev
  • fl_last
  • sentinel

Major GC is different in multicore.

  • caml_gc_phase
  • caml_gc_subphase
  • caml_gc_sweep_hp
  • caml_gc_clock
  • caml_heap_start
  • caml_major_ring
  • caml_major_ring_index
  • caml_major_window
  • caml_major_work_credit
  • gray_vals
  • gray_vals_cur
  • gray_vals_end
  • gray_vals_size
  • heap_is_pure
  • caml_allocated_words
  • caml_dependent_size
  • caml_dependent_allocated
  • caml_extra_heap_resources
  • markhp
  • chunk
  • limit
  • p_backlog
  • caml_allocation_policy

Runtime parameters apply to all the domains:

  • caml_parser_trace
  • caml_percent_free
  • caml_percent_max
  • caml_major_heap_increment
  • caml_init_custom_major_ratio
  • caml_init_custom_minor_max_bsz
  • caml_init_custom_minor_ratio
  • caml_init_heap_chunk_sz
  • caml_init_heap_wsz
  • caml_init_major_window
  • caml_init_max_percent_free
  • caml_init_max_stack_wsz
  • caml_init_minor_heap_wsz
  • caml_init_percent_free
  • caml_cleanup_on_exit
  • caml_trace_level
  • caml_verb_gc
  • caml_use_huge_pages
  • caml_custom_major_ratio
  • caml_custom_minor_max_bsz
  • caml_custom_minor_ratio
  • caml_runtime_warnings
  • caml_runtime_warnings_first

The details of interaction of profilers and multi-threaded execution has not been worked out yet. If the profiles can be collected per domain and collated later, then we can move these variables under domain state.

  • caml_memprof_postponed_head
  • caml_memprof_suspended
  • caml_memprof_young_trigger
  • caml_spacetime_trie_node_ptr
  • caml_spacetime_finaliser_trie_root
  • caml_extern_allow_out_of_heap

I can't see this variable used in the runtime.

  • caml_huge_fallback_count

Process level constants.

  • caml_code_area_end
  • caml_code_area_start
  • caml_exe_name
  • caml_system__frametable
  • frametables
  • caml_frame_descriptors
  • caml_frame_descriptors_mask
  • caml_int32_ops
  • caml_nativeint_ops
  • caml_int64_ops
  • caml_ba_ops
  • caml_code_fragments_table
  • caml_atom_table
  • caml_array_bound_error_exn
  • caml_ba_element_size
  • caml_debug_info

Signals are handled at process-level. Any domain may be interrupted to deliver the signal.

  • caml_async_signal_mode
  • caml_pending_signals
  • caml_signal_handlers
  • caml_signals_are_pending

No page tables in multicore.

  • caml_page_table

The debugger will only work for single-threaded execution due to fork.

  • caml_debugger_fork_mode
  • caml_debugger_in_use
  • caml_event_count

Multicore uses a different ephemeron implementation and the structures are different. Not moving existing structures now.

  • caml_ephe_list_head
  • caml_ephe_none
  • ephe_dummy
  • ephe_list_pure
  • ephes_checked_if_pure
  • ephes_to_check

--

The following items were added after @gasche pointed out that this list is not exhaustive.

Marshaling has (various problems in multicore)[https://github.com/ocaml-multicore/ocaml-multicore/issues/75]. Since the design isn't final yet, I have not yet moved the following variables to the domain state.

  • extern_flags
  • extern_limit
  • extern_output_block
  • extern_output_first
  • extern_ptr
  • extern_stack_init
  • extern_trail_block
  • extern_trail_cur
  • extern_trail_first
  • extern_trail_limit
  • extern_userprovided_output
  • intern_block
  • intern_color
  • intern_dest
  • intern_extra_block
  • intern_header
  • intern_input
  • intern_obj_table
  • intern_src
  • intern_stack_init
@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 12, 2019

The domain state now works on Linux+arm64 and Linux+power. This PR should also work on power (32bit), but is untested. On arm64 and power, there are enough registers available. Hence, we use the callee saved register r28 on power and x25 on arm64 for the domain state pointer instead of taking over exception pointer register (r14 in amd64). Obviously, r28 on power and x25 on arm64 are not available for register allocation.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 15, 2019

Implemented support for i386. Tested Windows (Cygwin) + x86-64. Cygwin x86 should work since there are not windows specific things in the i386 port. Also tested on FreeBSD x86-64.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 17, 2019

arm is now supported.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 17, 2019

s390x now works.

@kayceesrk kayceesrk marked this pull request as ready for review Jun 17, 2019
@kayceesrk kayceesrk force-pushed the kayceesrk:r14-globals branch from 92aa0aa to 75a9ba0 Jun 17, 2019
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Jun 18, 2019

I see the AppVeyor failure with Visual Studio 2015 but not with Visual Studio 2017. This patch appears to resolve it for VS2015:

diff --git a/runtime/caml/misc.h b/runtime/caml/misc.h
index 9164d1098..e11b1673f 100644
--- a/runtime/caml/misc.h
+++ b/runtime/caml/misc.h
@@ -125,7 +125,7 @@ extern caml_timing_hook caml_minor_gc_begin_hook, caml_minor_gc_end_hook;
 extern caml_timing_hook caml_finalise_begin_hook, caml_finalise_end_hook;

 #define CAML_STATIC_ASSERT_3(b, l) \
-  typedef CAMLunused_start \
+  CAMLunused_start typedef \
     char static_assertion_failure_line_##l[(b) ? 1 : -1] CAMLunused_end
 #define CAML_STATIC_ASSERT_2(b, l) CAML_STATIC_ASSERT_3(b, l)
 #define CAML_STATIC_ASSERT(b) CAML_STATIC_ASSERT_2(b, __LINE__)

however both VS2015 and VS2017 eventually hit:

OCAML_FLEXLINK="./boot/ocamlrun ./flexdll/flexlink.exe" ./boot/ocamlrun ./ocamlopt -g -nostdlib -I stdlib -I otherlibs/dynlink  -o ocamlc.opt compilerlibs/ocamlcommon.cmxa compilerlibs/ocamlbytecomp.cmxa driver/main.cmx -cclib "advapi32.lib ws2_32.lib version.lib"
** Cannot resolve symbols for stdlib\libasmrun.lib(callback_n.obj):
 caml_callback2_asm
 caml_callback3_asm
 caml_callback_asm
** Cannot resolve symbols for stdlib\libasmrun.lib(amd64nt.obj):
 caml_backtrace_active
 caml_backtrace_pos
 caml_bottom_of_stack
 caml_exception_pointer
 caml_gc_regs
 caml_last_return_address
 caml_young_limit
 caml_young_ptr
File "caml_startup", line 1:
Error: Error during linking
make[2]: *** [Makefile:885: ocamlc.opt] Error 2
make[2]: Leaving directory '/cygdrive/c/Users/DRA/Documents/ocaml-multicore'
make[1]: *** [Makefile:424: opt.opt] Error 2
make[1]: Leaving directory '/cygdrive/c/Users/DRA/Documents/ocaml-multicore'
make: *** [Makefile:475: world.opt] Error 2

The changes to amd64.S need applying to amd64nt.asm

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 21, 2019

msvc64 now works. The CI tests are now passing.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 21, 2019

@alainfrisch @chambart Would be interested in hearing our thoughts on the PR.

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Jun 21, 2019

Copy link
Contributor

dra27 left a comment

Build system and copyright headers - I haven't reviewed the technical details.

Makefile Outdated Show resolved Hide resolved
runtime/caml/domain.h Outdated Show resolved Hide resolved
runtime/caml/domain_state.h Outdated Show resolved Hide resolved
runtime/caml/domain.h Outdated Show resolved Hide resolved
runtime/caml/domain_state.tbl Outdated Show resolved Hide resolved
utils/domainstate.ml.c Outdated Show resolved Hide resolved
utils/domainstate.mli.c Show resolved Hide resolved
Makefile Show resolved Hide resolved
runtime/Makefile Outdated Show resolved Hide resolved
@@ -235,10 +235,6 @@
/* **** meta.c */

/* **** minor_gc.c */

This comment has been minimized.

Copy link
@dra27

dra27 Jun 21, 2019

Contributor

Is this huge change the appropriate time to consider removing compatibility.h (or at least making its inclusion a preprocessor error)?

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 26, 2019

Member

It may be the right time, but this should be a separate PR, that we'll need to check on OPAM and be ready to revert.

This comment has been minimized.

Copy link
@kayceesrk

kayceesrk Jun 27, 2019

Author Contributor

ugh. The macro definitions such as #define young_ptr .... interferes with the field with the same name in domain state. If we are not modifying compatibitlity.h in this PR, then we should rename the domain state fields (young_start, young_end, young_ptr, young_limit, local_roots) to something else (by perhaps adding a prefix). Any suggestions on this?

This comment has been minimized.

Copy link
@kayceesrk

kayceesrk Jun 27, 2019

Author Contributor

I reverted the change to compatibility.h. Now the PR doesn't modify compatibility.h.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 24, 2019

Precheck passes: https://ci.inria.fr/ocaml/job/precheck/275/

The one failing test on mingw32 is the random number generation test which is expected to fail every 10000 runs [0].

[0] https://github.com/ocaml/ocaml/blob/trunk/testsuite/tests/lib-random/rand.ml

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Jun 24, 2019

All prechecks are successful https://ci.inria.fr/ocaml/job/precheck/276/

@kayceesrk kayceesrk mentioned this pull request Jun 24, 2019
Changes Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
kayceesrk added 9 commits Jun 27, 2019
The domain state structure (caml_domain_state) uses certaing field names
(young_start, young_limit, etc.) that conflict with the macro
definitions in compatibility.h that use the same names. These macro
definitions are made only when CAML_NAME_SPACE is not defined. To avoid
conflict, when CAML_NAME_SPACE is not defined, we prefix the field names
in caml_domain_state with an underscore.

Update .depend files.
@kayceesrk kayceesrk force-pushed the kayceesrk:r14-globals branch from 239a667 to ad96dec Aug 23, 2019
@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Aug 23, 2019

I have implemented the change suggested by @dra27 to fix the incompatibilities with compatibility.h in ad96dec.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Aug 24, 2019

Mainly for the record, as @kayceesrk and I have been discussing the last bits offline.

I have a silly test program which provides a primitive for retrieving caml_young_limit as a nativeint which can be built both with and without CAML_NAME_SPACE which I've used to test the compatibility of this. I will try to convert this to an ocamltest shortly.

Compatibility definitions for caml_young_start, etc. are now automatically loaded in user code only if the appropriate header (memory.h or minor_gc.h) is #included. Note that this only works outside the compiler source tree (i.e. the compiler codebase itself cannot use these compatibility aliases, but all user code can - we could in the future insert deprecation pragmas, I guess).

young_start and so forth works now when CAML_NAME_SPACE is not defined because all the fields in Caml_state then begin with an underscore (this is fine, as the struct is otherwise identical). So when CAML_NAME_SPACE is undefined, compatibility.h is able to #define young_start to Caml_state->_young_start and avoid a recursive definition problem. Fortunately, this ugliness is restricted to the headers only, and a new macro Caml_state_field means that in the headers we now just write Caml_state_field(young_start).

@dra27
dra27 approved these changes Aug 24, 2019
Copy link
Contributor

dra27 left a comment

It's making one final pass through precheck. Assuming it passes, I see no reason for @stedolan not to push the appropriate merge button...

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Aug 26, 2019

The newly added test breaks under MacOS. @dra27 is looking at it.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Aug 26, 2019

It's just completing precheck#294, but the macOS tests are already blue, so hopefully it's correct this time!

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Aug 27, 2019

Looks like precheck#294 has succeeded. Any thoughts on this @stedolan before pressing the merge button?

@stedolan

This comment has been minimized.

Copy link
Contributor

stedolan commented Aug 27, 2019

Looks great! Very happy to see this finally getting in.

@stedolan stedolan merged commit 5ad6430 into ocaml:trunk Aug 27, 2019
2 checks passed
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

gasche commented Aug 29, 2019

While looking at #8729, I wondered why the global variables in intern.c (describing the marshaller state) are not encapsulated in the domain structure as well. Is this intentional? I don't see any of the intern_* static variables listed in @kayceesrk's very helpful table of unchanged globals.

@kayceesrk

This comment has been minimized.

Copy link
Contributor Author

kayceesrk commented Aug 31, 2019

@gasche This looks like an oversight. Thanks for bringing this up. Marshaling has various problems in multicore (ocaml-multicore/ocaml-multicore#75). Since the design isn't final yet, in multicore, we had simply made the globals in intern.c thread-local (https://github.com/ocaml-multicore/ocaml-multicore/blob/master/byterun/intern.c#L38-L43). I shall add the global variables in intern.c to the table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.