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.

Sign up for GitHub

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

Jump to bottom

Enable GC_set_handle_fork(1) for Darwin with GC_incremental on #103

Closed
ivmai opened this issue May 16, 2016 · 20 comments
Closed

Enable GC_set_handle_fork(1) for Darwin with GC_incremental on #103

ivmai opened this issue May 16, 2016 · 20 comments
Labels
feature-request

Comments

@ivmai
Copy link
Owner

ivmai commented May 16, 2016

Both single- and multi-threaded modes should be handled.
@ivmai ivmai mentioned this issue Aug 19, 2016
Closed
@ivmai ivmai changed the title Enable GC_set_handle_fork(1) for Darwin with GC_dirty_maintained on [Feature] Enable GC_set_handle_fork(1) for Darwin with GC_incremental on Jul 18, 2017
@ivmai ivmai mentioned this issue Sep 26, 2023
Open
ivmai added a commit that referenced this issue Nov 30, 2023
@ivmai
Turn on handle fork by default on Darwin (multi-threaded only)
ba2861e 
Issue #103 (bdwgc).

Remove mutual exclusion between fork handling and GC incremental mode
on Darwin in a multi-threaded configuration.

Note: the incremental mode is turned off in the child process after
fork, for now.

* CMakeLists.txt [enable_threads && CMAKE_USE_PTHREADS_INIT && !WIN32
&& enable_handle_fork && !disable_handle_fork] (HANDLE_FORK): Define
C macro even if APPLE; remove comment.
* configure.ac [$enable_handle_fork!=yes && $enable_handle_fork!=no
&& $enable_handle_fork!=manual && $THREADS==posix
&& $host==*-*-darwin*] (HANDLE_FORK): Define AC macro; remove comment.
* tests/gctest.c [DARWIN && MPROTECT_VDB && !MAKE_BACK_GRAPH
&& !TEST_HANDLE_FORK && !NO_TEST_HANDLE_FORK] (NO_TEST_HANDLE_FORK): Do
not define if THREADS.
* include/private/gc_priv.h [CAN_HANDLE_FORK && MPROTECT_VDB
&& GC_DARWIN_THREADS] (GC_dirty_update_child): Declare as function
(instead of a no-op macro).
* pthread_support.c [CAN_HANDLE_FORK && !GC_DISABLE_INCREMENTAL]
(fork_child_proc): Move GC_dirty_update_child() call upper to be right
after GC_release_dirty_lock() one.
* pthread_support.c [CAN_HANDLE_FORK && GC_DARWIN_THREADS
&& MPROTECT_VDB] (GC_atfork_prepare): Remove assertion (about
GC_handle_fork) and abort.
* os_dep.c [DARWIN && MPROTECT_VDB && CAN_HANDLE_FORK && THREADS]
(GC_dirty_update_child): Implement (unprotect the entire heap, restore
the old task exception ports and turn off the incremental mode).
* os_dep.c [DARWIN && MPROTECT_VDB && THREADS] (GC_mprotect_thread):
Reformat code.
* os_dep.c [DARWIN && MPROTECT_VDB && CAN_HANDLE_FORK] (GC_dirty_init):
Do not WARN and return FALSE if THREADS and GC_handle_fork; add
assertion that me variable is non-zero.
* os_dep.c [DARWIN && MPROTECT_VDB] (GC_forward_exception): Cast 1 to
exception_mask_t before left-shift by exception; reformat code.
@ivmai ivmai mentioned this issue Nov 30, 2023
Open
@ivmai
Copy link
Owner Author

ivmai commented Nov 30, 2023 •
edited
Loading

The issue is resolved (by commit ba2861e) only on multi-threaded builds. Also, the incremental mode is turned off in the child process. See issues #590 and #591.

@ivmai ivmai closed this as completed Nov 30, 2023
@ivmai ivmai mentioned this issue Nov 30, 2023
Open
@catap
Copy link
Contributor

catap commented Dec 13, 2023

@ivmai this seems like significant improvement for macOS at ECL usage.

Without it I can reproduce random failure like this:

0   libsystem_kernel.dylib        	    0x7ff817ed6ffe __pthread_kill + 10
1   libsystem_pthread.dylib       	    0x7ff817f0d1ff pthread_kill + 263
2   libsystem_c.dylib             	    0x7ff817e58d14 abort + 123
3   libgc.1.dylib                 	       0x10d153eb8 GC_stop_world.cold.1 + 25
4   libgc.1.dylib                 	       0x10d152786 GC_stop_world + 232
5   libgc.1.dylib                 	       0x10d1425d7 GC_stopped_mark + 70
6   libgc.1.dylib                 	       0x10d142494 GC_try_to_collect_inner + 311
7   libgc.1.dylib                 	       0x10d1436c5 GC_collect_or_expand + 189
8   libgc.1.dylib                 	       0x10d143947 GC_allocobj + 265
9   libgc.1.dylib                 	       0x10d148155 GC_generic_malloc_inner + 253
10  libgc.1.dylib                 	       0x10d1483e5 GC_generic_malloc + 83
11  libecl.23.9.9.dylib           	       0x10d581b0f ecl_alloc + 32

Any plan to release the new version with this changes?

I've used 98b5d38 as local root for my tests.

@catap
Copy link
Contributor

catap commented Dec 13, 2023

Anyway, it doesn't fix #569

@ivmai
Copy link
Owner Author

ivmai commented Dec 13, 2023

Yes, that's a different issue.

@catap
Copy link
Contributor

catap commented Dec 13, 2023

and doesn't fix sharplispers/ironclad#63 which is expected

@ivmai
Copy link
Owner Author

ivmai commented Dec 13, 2023

and doesn't fix sharplispers/ironclad#63 which is expected

Is there an issue here?

@catap
Copy link
Contributor

catap commented Dec 13, 2023

and doesn't fix sharplispers/ironclad#63 which is expected

Is there an issue here?

By some unknown reason when ECL uses clang it leads to random result of hashing, and switching to GCC fixes it.

@ivmai
Copy link
Owner Author

ivmai commented Dec 13, 2023

Without it I can reproduce random failure like this:
0x7ff817e58d14 abort + 123
3 libgc.1.dylib 0x10d153eb8 GC_stop_world.cold.1 + 25

I understand.

Any plan to release the new version with this changes?

It's planned at the end of Feb. v8.4.0

@catap
Copy link
Contributor

catap commented Dec 13, 2023

It's planned at the end of Feb. v8.4.0

Do you think I can backport only ba2861e into last relesead version to increase stability ECL at MacPorts? Is it enough?

@ivmai
Copy link
Owner Author

ivmai commented Dec 13, 2023

Do you think I can backport only ba2861e into last relesead version to increase stability ECL at MacPorts? Is it enough?

Initially I though of it as a feature (to enable automatic fork handling by default), but now I see it serves as a fix... Let me check the complexity of backporting to the stable branch.

@catap catap mentioned this issue Dec 13, 2023
Merged
8 tasks
ivmai added a commit that referenced this issue Dec 14, 2023
@ivmai
Turn on handle fork by default on Darwin (multi-threaded only)
47e7999 
(a cherry-pick of commit ba2861e from 'master')

Issue #103 (bdwgc).

Remove mutual exclusion between fork handling and GC incremental mode
on Darwin in a multi-threaded configuration.

Note: the incremental mode is turned off in the child process after
fork, for now.

* CMakeLists.txt [CMAKE_USE_PTHREADS_INIT && !WIN32
&& enable_handle_fork && !disable_handle_fork] (HANDLE_FORK): Define
C macro even if APPLE; remove comment.
* configure.ac [$enable_handle_fork!=yes && $enable_handle_fork!=no
&& $enable_handle_fork!=manual && $THREADS==posix
&& $host==*-*-darwin*] (HANDLE_FORK): Define AC macro; remove comment.
* tests/test.c [DARWIN && MPROTECT_VDB && !MAKE_BACK_GRAPH
&& !TEST_HANDLE_FORK && !NO_TEST_HANDLE_FORK] (NO_TEST_HANDLE_FORK): Do
not define if THREADS.
* include/private/gc_priv.h [CAN_HANDLE_FORK && MPROTECT_VDB
&& GC_DARWIN_THREADS] (GC_dirty_update_child): Declare as function
(instead of a no-op macro).
* pthread_support.c [CAN_HANDLE_FORK && !GC_DISABLE_INCREMENTAL]
(fork_child_proc): Move GC_dirty_update_child() call upper to be right
after GC_release_dirty_lock() one.
* pthread_support.c [CAN_HANDLE_FORK && GC_DARWIN_THREADS
&& MPROTECT_VDB] (GC_atfork_prepare): Remove assertion (about
GC_handle_fork) and abort.
* os_dep.c [DARWIN && MPROTECT_VDB && CAN_HANDLE_FORK && THREADS]
(GC_dirty_update_child): Implement (unprotect the entire heap, restore
the old task exception ports and turn off the incremental mode).
* os_dep.c [DARWIN && MPROTECT_VDB && THREADS] (GC_mprotect_thread):
Reformat code.
* os_dep.c [DARWIN && MPROTECT_VDB && CAN_HANDLE_FORK] (GC_dirty_init):
Do not WARN and return FALSE if THREADS and GC_handle_fork; add
assertion that me variable is non-zero.
* os_dep.c [DARWIN && MPROTECT_VDB] (GC_forward_exception): Cast 1 to
exception_mask_t before left-shift by exception; reformat code.
@ivmai
Copy link
Owner Author

ivmai commented Dec 14, 2023

Backported, please test branch release-8_2.

@catap
Copy link
Contributor

catap commented Dec 14, 2023

I can't reproduce this issue on my laptop anymore on da06919.

Run tests via github CI with gc from v8.2.4 and da06919. Expected that the first fails, and the second one passes. ETA couple of hours.

@catap
Copy link
Contributor

catap commented Dec 14, 2023

Confirmed with GitHub CI. Fails with v8.2.4 and works with stable from da06919

@catap
Copy link
Contributor

catap commented Dec 14, 2023

Can you make 8.2.5 release with this huge improvement for macOS?

@ivmai
Copy link
Owner Author

ivmai commented Dec 14, 2023

Can you make 8.2.5 release with this huge improvement for macOS?

v8.2.6. I will think of.

@catap
Copy link
Contributor

catap commented Dec 14, 2023

why v8.2.6? I haven't see v8.2.5 here https://github.com/ivmai/bdwgc/tags

@ivmai
Copy link
Owner Author

ivmai commented Dec 14, 2023

why v8.2.6? I haven't see v8.2.5 here https://github.com/ivmai/bdwgc/tags

All micro numbers are even ;) just a convention.

@ivmai
Copy link
Owner Author

ivmai commented Dec 14, 2023

Can you make 8.2.5 release with this huge improvement for macOS?

I want to fix another ancient bug on macOS before the release - issue #178

@catap
Copy link
Contributor

catap commented Dec 14, 2023

Fair enough.

@catap catap mentioned this issue Jan 9, 2024
Closed
12 tasks
@ivmai
Copy link
Owner Author

ivmai commented Feb 5, 2024

Can you make 8.2.5 release with this huge improvement for macOS?

Released: https://github.com/ivmai/bdwgc/releases/tag/v8.2.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@catap @ivmai