From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Pip Cet via "Emacs development discussions." Newsgroups: gmane.emacs.devel Subject: Re: Some experience with the igc branch Date: Mon, 23 Dec 2024 16:03:53 +0000 Message-ID: <87pllicrpi.fsf@protonmail.com> References: <87o713wwsi.fsf@telefonica.net> <87ldw7fwet.fsf@protonmail.com> <87a5cnfj8t.fsf@protonmail.com> <86seqe4j4f.fsf@gnu.org> <87ttaucub8.fsf@protonmail.com> Reply-To: Pip Cet Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="16552"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Eli Zaretskii , ofv@wanadoo.es, emacs-devel@gnu.org, eller.helmut@gmail.com, acorallo@gnu.org To: =?utf-8?Q?Gerd_M=C3=B6llmann?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Dec 23 17:36:23 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1tPlPq-00045Y-OG for ged-emacs-devel@m.gmane-mx.org; Mon, 23 Dec 2024 17:36:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1tPlPQ-0004uv-4e; Mon, 23 Dec 2024 11:35:56 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tPkua-0006Jq-3P for emacs-devel@gnu.org; Mon, 23 Dec 2024 11:04:04 -0500 Original-Received: from mail-40131.protonmail.ch ([185.70.40.131]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tPkuX-0001Jb-LD for emacs-devel@gnu.org; Mon, 23 Dec 2024 11:04:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1734969838; x=1735229038; bh=CM9TXBSn77en2OSdCqT98jweS/87lTT/x5ys28VASt0=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=Yeb4Sb8ugC/LKgJdQtkcDFiB5LGZcAWxetlOkf1rYKF0p4qINLys64cyVeyuBcJ2g NmYfHpBeds5Jj1FWXAj2NBg4SLpR5MecRcGKTeCkTCuCXL6IVXJNVNO7qt3HFDzKDF i7cQZPP9F6T9ZiIO/QNVamjddiolgj+Er3pvQd2RP1g+dD6GZkiKaRt6GHz3DCoRAA FdxsPQidfrYwoD9RwgppJy8m+Rb25RuqRM7RfEssBWGC2NOh33BJrJegezAB1O+V2C Igm2kbZmlsVQBpV2zd4eN0BTd1DpogOpR32cQnjihqmQAxQoN46tNaLjkyv+tS+GIe 46VdgKmCGOkpg== In-Reply-To: Feedback-ID: 112775352:user:proton X-Pm-Message-ID: c6e62b99e90a1d54a5fcdb49dd1224d7e43399d3 Received-SPF: pass client-ip=185.70.40.131; envelope-from=pipcet@protonmail.com; helo=mail-40131.protonmail.ch X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, RCVD_IN_VALIDITY_SAFE_BLOCKED=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Mon, 23 Dec 2024 11:35:47 -0500 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:326917 Archived-At: Gerd M=C3=B6llmann writes: > Pip Cet writes: > >> I'll spare you most of the details for now, but having read the mps >> header, MPS allocation is not safe to use from separate threads without >> locking the AP (or having per-thread APs), which we might end up doing >> on Windows, IIRC. > > Now I'm confused. We're using thread allocation points. See > create_thread_aps, thread_ap, and so on. I was confused. This is only a problem if we allocate memory from a signal handler, which is effectively sharing the per-thread structure. (I'm still confused. My patch worked on the first attempt, which my code never does. I suspect that while I made a mistake, it caused a subtle bug rather than an obvious one.) And we don't want to allocate memory from signal handlers, right? We could, now (see warnings below): diff --git a/src/igc.c b/src/igc.c index eb72406e529..14ecc30f982 100644 --- a/src/igc.c +++ b/src/igc.c @@ -747,19 +747,41 @@ IGC_DEFINE_LIST (igc_root); =20 /* Registry entry for an MPS thread mps_thr_t. */ =20 +#include +#include + +struct emacs_ap +{ + mps_ap_t mps_ap; + struct igc *gc; + pthread_t allocation_thread; + atomic_uintptr_t usable_memory; + atomic_uintptr_t usable_bytes; + + atomic_uintptr_t waiting_threads; + atomic_uintptr_t requested_bytes; + atomic_intptr_t requested_type; +}; + +typedef struct emacs_ap emacs_ap_t; + +#ifndef ATOMIC_POINTER_LOCK_FREE +#error "this probably won't work" +#endif + struct igc_thread { struct igc *gc; mps_thr_t thr; =20 /* Allocation points for the thread. */ - mps_ap_t dflt_ap; - mps_ap_t leaf_ap; - mps_ap_t weak_strong_ap; - mps_ap_t weak_weak_ap; - mps_ap_t weak_hash_strong_ap; - mps_ap_t weak_hash_weak_ap; - mps_ap_t immovable_ap; + emacs_ap_t dflt_ap; + emacs_ap_t leaf_ap; + emacs_ap_t weak_strong_ap; + emacs_ap_t weak_weak_ap; + emacs_ap_t weak_hash_strong_ap; + emacs_ap_t weak_hash_weak_ap; + emacs_ap_t immovable_ap; =20 /* Quick access to the roots used for specpdl, bytecode stack and control stack. */ @@ -805,6 +827,8 @@ IGC_DEFINE_LIST (igc_thread); =20 /* Registered threads. */ struct igc_thread_list *threads; + + pthread_cond_t cond; }; =20 static bool process_one_message (struct igc *gc); @@ -2904,8 +2928,84 @@ igc_root_destroy_comp_unit_eph (struct Lisp_Native_C= omp_Unit *u) maybe_destroy_root (&u->data_eph_relocs_root); } =20 +static mps_addr_t alloc_impl_raw (size_t size, enum igc_obj_type type, mps= _ap_t ap); +static mps_addr_t alloc_impl (size_t size, enum igc_obj_type type, emacs_a= p_t *ap); + +static void *igc_allocation_thread (void *ap_v) +{ + emacs_ap_t *ap =3D ap_v; + while (true) + { + if (ap->requested_bytes) +=09{ +=09 void *p =3D alloc_impl_raw (ap->requested_bytes, (enum igc_obj_type) = ap->requested_type, ap->mps_ap); +=09 atomic_store (&ap->usable_memory, (uintptr_t) p); +=09 atomic_store (&ap->usable_bytes, ap->requested_bytes); +=09 atomic_store (&ap->requested_type, -1); +=09 atomic_store (&ap->requested_bytes, 0); +=09} + } + + return NULL; +} + +static mps_addr_t alloc_impl (size_t size, enum igc_obj_type type, emacs_a= p_t *ap) +{ + if (size =3D=3D 0) + return 0; + while (true) + { + uintptr_t other_threads =3D atomic_fetch_add (&ap->waiting_threads, = 1); + if (other_threads !=3D 0) +=09{ +=09 /* we know that the other "thread" is actually on top of us, +=09 * and we're a signal handler. Wait, should we even be +=09 * allocating memory? We should still eassert that we're the +=09 * right thread. */ +=09 emacs_ap_t saved_state; +=09 while (ap->requested_bytes); +=09 memcpy (&saved_state, ap, sizeof saved_state); +=09 atomic_store (&ap->waiting_threads, 0); +=09 mps_addr_t ret =3D alloc_impl (size, type, ap); +=09 atomic_store (&ap->waiting_threads, saved_state.waiting_threads); +=09 memcpy (ap, &saved_state, sizeof saved_state); +=09 atomic_fetch_add (&ap->waiting_threads, -1); +=09 return ret; +=09} + + atomic_store (&ap->requested_type, (uintptr_t) type); + atomic_store (&ap->requested_bytes, (uintptr_t) size); + + while (ap->requested_bytes); + + mps_addr_t ret =3D (mps_addr_t) ap->usable_memory; + atomic_fetch_add (&ap->waiting_threads, -1); + return ret; + } +} + +static mps_res_t emacs_ap_create_k (emacs_ap_t *ap, mps_pool_t pool, +=09=09=09=09 mps_arg_s *args) +{ + atomic_store(&ap->usable_memory, 0); + atomic_store(&ap->usable_bytes, 0); + atomic_store(&ap->waiting_threads, 0); + atomic_store(&ap->requested_bytes, 0); + + pthread_attr_t thread_attr; + pthread_attr_init (&thread_attr); + pthread_create(&ap->allocation_thread, &thread_attr, igc_allocation_thre= ad, ap); + + return mps_ap_create_k (&ap->mps_ap, pool, args); +} + +static void emacs_ap_destroy (emacs_ap_t *ap) +{ + return; +} + static mps_res_t -create_weak_ap (mps_ap_t *ap, struct igc_thread *t, bool weak) +create_weak_ap (emacs_ap_t *ap, struct igc_thread *t, bool weak) { struct igc *gc =3D t->gc; mps_res_t res; @@ -2914,14 +3014,14 @@ create_weak_ap (mps_ap_t *ap, struct igc_thread *t,= bool weak) { MPS_ARGS_ADD (args, MPS_KEY_RANK, =09=09 weak ? mps_rank_weak () : mps_rank_exact ()); - res =3D mps_ap_create_k (ap, pool, args); + res =3D emacs_ap_create_k (ap, pool, args); } MPS_ARGS_END (args); return res; } =20 static mps_res_t -create_weak_hash_ap (mps_ap_t *ap, struct igc_thread *t, bool weak) +create_weak_hash_ap (emacs_ap_t *ap, struct igc_thread *t, bool weak) { struct igc *gc =3D t->gc; mps_res_t res; @@ -2930,7 +3030,7 @@ create_weak_hash_ap (mps_ap_t *ap, struct igc_thread = *t, bool weak) { MPS_ARGS_ADD (args, MPS_KEY_RANK, =09=09 weak ? mps_rank_weak () : mps_rank_exact ()); - res =3D mps_ap_create_k (ap, pool, args); + res =3D emacs_ap_create_k (ap, pool, args); } MPS_ARGS_END (args); return res; @@ -2940,12 +3040,15 @@ create_weak_hash_ap (mps_ap_t *ap, struct igc_threa= d *t, bool weak) create_thread_aps (struct igc_thread *t) { struct igc *gc =3D t->gc; + pthread_condattr_t condattr; + pthread_condattr_init (&condattr); + pthread_cond_init (&gc->cond, &condattr); mps_res_t res; - res =3D mps_ap_create_k (&t->dflt_ap, gc->dflt_pool, mps_args_none); + res =3D emacs_ap_create_k (&t->dflt_ap, gc->dflt_pool, mps_args_none); IGC_CHECK_RES (res); - res =3D mps_ap_create_k (&t->leaf_ap, gc->leaf_pool, mps_args_none); + res =3D emacs_ap_create_k (&t->leaf_ap, gc->leaf_pool, mps_args_none); IGC_CHECK_RES (res); - res =3D mps_ap_create_k (&t->immovable_ap, gc->immovable_pool, mps_args_= none); + res =3D emacs_ap_create_k (&t->immovable_ap, gc->immovable_pool, mps_arg= s_none); IGC_CHECK_RES (res); res =3D create_weak_ap (&t->weak_strong_ap, t, false); res =3D create_weak_hash_ap (&t->weak_hash_strong_ap, t, false); @@ -3007,13 +3110,13 @@ igc_thread_remove (void **pinfo) destroy_root (&t->d.stack_root); destroy_root (&t->d.specpdl_root); destroy_root (&t->d.bc_root); - mps_ap_destroy (t->d.dflt_ap); - mps_ap_destroy (t->d.leaf_ap); - mps_ap_destroy (t->d.weak_strong_ap); - mps_ap_destroy (t->d.weak_weak_ap); - mps_ap_destroy (t->d.weak_hash_strong_ap); - mps_ap_destroy (t->d.weak_hash_weak_ap); - mps_ap_destroy (t->d.immovable_ap); + emacs_ap_destroy (&t->d.dflt_ap); + emacs_ap_destroy (&t->d.leaf_ap); + emacs_ap_destroy (&t->d.weak_strong_ap); + emacs_ap_destroy (&t->d.weak_weak_ap); + emacs_ap_destroy (&t->d.weak_hash_strong_ap); + emacs_ap_destroy (&t->d.weak_hash_weak_ap); + emacs_ap_destroy (&t->d.immovable_ap); mps_thread_dereg (deregister_thread (t)); } =20 @@ -3677,7 +3780,7 @@ igc_on_idle (void) } } =20 -static mps_ap_t +static emacs_ap_t * thread_ap (enum igc_obj_type type) { struct igc_thread_list *t =3D current_thread->gc_info; @@ -3698,13 +3801,13 @@ thread_ap (enum igc_obj_type type) emacs_abort (); =20 case IGC_OBJ_MARKER_VECTOR: - return t->d.weak_weak_ap; + return &t->d.weak_weak_ap; =20 case IGC_OBJ_WEAK_HASH_TABLE_WEAK_PART: - return t->d.weak_hash_weak_ap; + return &t->d.weak_hash_weak_ap; =20 case IGC_OBJ_WEAK_HASH_TABLE_STRONG_PART: - return t->d.weak_hash_strong_ap; + return &t->d.weak_hash_strong_ap; =20 case IGC_OBJ_VECTOR: case IGC_OBJ_CONS: @@ -3719,12 +3822,12 @@ thread_ap (enum igc_obj_type type) case IGC_OBJ_FACE_CACHE: case IGC_OBJ_BLV: case IGC_OBJ_HANDLER: - return t->d.dflt_ap; + return &t->d.dflt_ap; =20 case IGC_OBJ_STRING_DATA: case IGC_OBJ_FLOAT: case IGC_OBJ_BYTES: - return t->d.leaf_ap; + return &t->d.leaf_ap; } emacs_abort (); } @@ -3796,7 +3899,7 @@ igc_hash (Lisp_Object key) object. */ =20 static mps_addr_t -alloc_impl (size_t size, enum igc_obj_type type, mps_ap_t ap) +alloc_impl_raw (size_t size, enum igc_obj_type type, mps_ap_t ap) { mps_addr_t p UNINIT; size =3D alloc_size (size); @@ -3845,7 +3948,7 @@ alloc (size_t size, enum igc_obj_type type) alloc_immovable (size_t size, enum igc_obj_type type) { struct igc_thread_list *t =3D current_thread->gc_info; - return alloc_impl (size, type, t->d.immovable_ap); + return alloc_impl (size, type, &t->d.immovable_ap); } =20 #ifdef HAVE_MODULES @@ -4883,17 +4986,17 @@ igc_on_pdump_loaded (void *dump_base, void *hot_sta= rt, void *hot_end, igc_alloc_dump (size_t nbytes) { igc_assert (global_igc->park_count > 0); - mps_ap_t ap =3D thread_ap (IGC_OBJ_CONS); + emacs_ap_t *ap =3D thread_ap (IGC_OBJ_CONS); size_t block_size =3D igc_header_size () + nbytes; mps_addr_t block; do { - mps_res_t res =3D mps_reserve (&block, ap, block_size); + mps_res_t res =3D mps_reserve (&block, ap->mps_ap, block_size); if (res !=3D MPS_RES_OK) =09memory_full (0); set_header (block, IGC_OBJ_INVALID, block_size, 0); } - while (!mps_commit (ap, block, block_size)); + while (!mps_commit (ap->mps_ap, block, block_size)); return (char *) block + igc_header_size (); } =20 Warnings: This is the "slow path" only, used for all allocations. Will cause a great number of busy-looping threads. Will be very slow. Creating additional emacs threads will result in a proportional number of additional threads, which will be very, very slow, so don't. Requires pthread.h and stdatomic.h, and still does things not covered by those APIs (memcpying over an atomic_uintptr_t, even if we know that its value won't change, is probably verboten, and definitely should be). I *think* this code might work if we allocate from signal handlers, and I think this code might work on systems that don't have lock-free atomics (once the #error is removed), but it definitely won't do both at the same time. Pip