#9075 closed bug (fixed)

Per-thread weak pointer list (remove global lock on mkWeak#)

Reported by: ezyang Owned by: ezyang
Priority: low Milestone:
Component: Runtime System Version: 7.9
Keywords: easy Cc: ekmett, simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

Currently, we have to take out the storage manager lock when a weak pointer is allocated. If you're making a lot of weak pointers, this could become a bottleneck. We should maintain a per-thread weak pointer list per generation, so that we don't have to take out a lock.

Marking priority as low since I don't think anyone actually is trying to allocate tons of weak pointers on multiple threads, but I think this optimization should be relatively cheap. Probably a good starter ticket.

Change History (9)

comment:1 Changed 11 months ago by ekmett

  • Cc ekmett added

comment:2 Changed 11 months ago by ezyang

  • Owner changed from simonmar to ezyang

comment:3 Changed 11 months ago by ezyang

Suggested patch, still validating:

From 28cb3e18051db7d858bd913c002aedb78630bcdb Mon Sep 17 00:00:00 2001
From: "Edward Z. Yang" <[email protected]>
Date: Thu, 29 May 2014 00:18:33 -0700
Subject: [PATCH] Per-capability nursery weak pointer lists.

Signed-off-by: Edward Z. Yang <[email protected]>
---
 rts/Capability.c                         |  2 ++
 rts/Capability.h                         |  5 +++++
 rts/PrimOps.cmm                          |  9 +++++----
 rts/RetainerProfile.c                    |  6 ++++++
 rts/RtsStartup.c                         |  5 ++++-
 rts/sm/GC.c                              |  1 +
 rts/sm/MarkWeak.c                        | 18 ++++++++++++++++++
 rts/sm/MarkWeak.h                        |  1 +
 utils/deriveConstants/DeriveConstants.hs |  2 ++
 9 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/rts/Capability.c b/rts/Capability.c
index 16b71b7..805a35b 100644
--- a/rts/Capability.c
+++ b/rts/Capability.c
@@ -273,6 +273,8 @@ initCapability( Capability *cap, nat i )
 	cap->mut_lists[g] = NULL;
     }
 
+    cap->weak_ptr_list_hd = NULL;
+    cap->weak_ptr_list_tl = NULL;
     cap->free_tvar_watch_queues = END_STM_WATCH_QUEUE;
     cap->free_invariant_check_queues = END_INVARIANT_CHECK_QUEUE;
     cap->free_trec_chunks = END_STM_CHUNK_LIST;
diff --git a/rts/Capability.h b/rts/Capability.h
index f342d92..d36d502 100644
--- a/rts/Capability.h
+++ b/rts/Capability.h
@@ -79,6 +79,11 @@ struct Capability_ {
     // full pinned object blocks allocated since the last GC
     bdescr *pinned_object_blocks;
 
+    // per-capability weak pointer list associated with nursery (older
+    // lists stored in generation object)
+    StgWeak *weak_ptr_list_hd;
+    StgWeak *weak_ptr_list_tl;
+
     // Context switch flag.  When non-zero, this means: stop running
     // Haskell code, and switch threads.
     int context_switch;
diff --git a/rts/PrimOps.cmm b/rts/PrimOps.cmm
index 1dc232d..84bcea5 100644
--- a/rts/PrimOps.cmm
+++ b/rts/PrimOps.cmm
@@ -577,10 +577,11 @@ stg_mkWeakzh ( gcptr key,
     StgWeak_finalizer(w)   = finalizer;
     StgWeak_cfinalizers(w) = stg_NO_FINALIZER_closure;
 
-    ACQUIRE_LOCK(sm_mutex);
-    StgWeak_link(w) = generation_weak_ptr_list(W_[g0]);
-    generation_weak_ptr_list(W_[g0]) = w;
-    RELEASE_LOCK(sm_mutex);
+    StgWeak_link(w) = Capability_weak_ptr_list_hd(MyCapability());
+    Capability_weak_ptr_list_hd(MyCapability()) = w;
+    if (Capability_weak_ptr_list_tl(MyCapability()) == NULL) {
+        Capability_weak_ptr_list_tl(MyCapability()) = w;
+    }
 
     IF_DEBUG(weak, ccall debugBelch(stg_weak_msg,w));
 
diff --git a/rts/RetainerProfile.c b/rts/RetainerProfile.c
index bdfc831..bfc9624 100644
--- a/rts/RetainerProfile.c
+++ b/rts/RetainerProfile.c
@@ -1781,6 +1781,12 @@ computeRetainerSet( void )
     //
     // The following code assumes that WEAK objects are considered to be roots
     // for retainer profilng.
+    for (n = 0; n < n_capabilities; n++) {
+        // NB: after a GC, all nursery weak_ptr_lists have been migrated
+        // to the global lists living in the generations
+        ASSERT(capabilities[n]->weak_ptr_list_hd == NULL);
+        ASSERT(capabilities[n]->weak_ptr_list_tl == NULL);
+    }
     for (g = 0; g < RtsFlags.GcFlags.generations; g++) {
         for (weak = generations[g].weak_ptr_list; weak != NULL; weak = weak->link) {
             // retainRoot((StgClosure *)weak);
diff --git a/rts/RtsStartup.c b/rts/RtsStartup.c
index 15e48a6..06e888c 100644
--- a/rts/RtsStartup.c
+++ b/rts/RtsStartup.c
@@ -304,7 +304,7 @@ hs_add_root(void (*init_root)(void) STG_UNUSED)
 static void
 hs_exit_(rtsBool wait_foreign)
 {
-    nat g;
+    nat g, i;
 
     if (hs_init_count <= 0) {
 	errorBelch("warning: too many hs_exit()s");
@@ -336,6 +336,9 @@ hs_exit_(rtsBool wait_foreign)
     exitScheduler(wait_foreign);
 
     /* run C finalizers for all active weak pointers */
+    for (i = 0; i < n_capabilities; i++) {
+        runAllCFinalizers(capabilities[i]->weak_ptr_list_hd);
+    }
     for (g = 0; g < RtsFlags.GcFlags.generations; g++) {
         runAllCFinalizers(generations[g].weak_ptr_list);
     }
diff --git a/rts/sm/GC.c b/rts/sm/GC.c
index d22a31e..2122385 100644
--- a/rts/sm/GC.c
+++ b/rts/sm/GC.c
@@ -378,6 +378,7 @@ GarbageCollect (nat collect_gen,
 #endif
 
   // Mark the weak pointer list, and prepare to detect dead weak pointers.
+  collectFreshWeakPtrs();
   markWeakPtrList();
   initWeakForGC();
 
diff --git a/rts/sm/MarkWeak.c b/rts/sm/MarkWeak.c
index af953cd..1a7359f 100644
--- a/rts/sm/MarkWeak.c
+++ b/rts/sm/MarkWeak.c
@@ -341,6 +341,24 @@ static void tidyThreadList (generation *gen)
     }
 }
 
+void collectFreshWeakPtrs()
+{
+    nat i;
+    generation *gen = &generations[0];
+    // move recently allocated weak_ptr_list to the old list as well
+    for (i = 0; i < n_capabilities; i++) {
+        Capability *cap = capabilities[i];
+        if (cap->weak_ptr_list_tl != NULL) {
+            cap->weak_ptr_list_tl->link = gen->weak_ptr_list;
+            gen->weak_ptr_list = cap->weak_ptr_list_hd;
+            cap->weak_ptr_list_tl = NULL;
+            cap->weak_ptr_list_hd = NULL;
+        } else {
+            ASSERT(cap->weak_ptr_list_hd == NULL);
+        }
+    }
+}
+
 /* -----------------------------------------------------------------------------
    Evacuate every weak pointer object on the weak_ptr_list, and update
    the link fields.
diff --git a/rts/sm/MarkWeak.h b/rts/sm/MarkWeak.h
index f9bacfa..bd0231d 100644
--- a/rts/sm/MarkWeak.h
+++ b/rts/sm/MarkWeak.h
@@ -20,6 +20,7 @@ extern StgWeak *old_weak_ptr_list;
 extern StgTSO *resurrected_threads;
 extern StgTSO *exception_threads;
 
+void    collectFreshWeakPtrs   ( void );
 void    initWeakForGC          ( void );
 rtsBool traverseWeakPtrList    ( void );
 void    markWeakPtrList        ( void );
diff --git a/utils/deriveConstants/DeriveConstants.hs b/utils/deriveConstants/DeriveConstants.hs
index d15f619..9bf2160 100644
--- a/utils/deriveConstants/DeriveConstants.hs
+++ b/utils/deriveConstants/DeriveConstants.hs
@@ -349,6 +349,8 @@ wanteds = concat
           ,structField C    "Capability" "context_switch"
           ,structField C    "Capability" "interrupt"
           ,structField C    "Capability" "sparks"
+          ,structField C    "Capability" "weak_ptr_list_hd"
+          ,structField C    "Capability" "weak_ptr_list_tl"
 
           ,structField Both "bdescr" "start"
           ,structField Both "bdescr" "free"
-- 
1.9.2


comment:4 Changed 11 months ago by simonmar

Patch looks good to me, nice work.

comment:5 Changed 11 months ago by ezyang

Posted patch fails memo001 test, investigating.

comment:6 Changed 11 months ago by Edward Z. Yang <ezyang@…>

In 723095b0e4c5838c7eefd757af56ab2a7c614801/ghc:

Per-capability nursery weak pointer lists, fixes #9075

Signed-off-by: Edward Z. Yang <[email protected]>

comment:7 Changed 11 months ago by ezyang

  • Status changed from new to merge

(The bug was collectFreshWeakPtrs was being called too late, after some scavenging had occurred).

I am indifferent to whether or not this feature goes in 7.8 stable, but maybe someone else wants to lobby for it, so I'll leave it as merge for now.

comment:8 Changed 11 months ago by ekmett

My needs aren't pressing. Knowing that a long term solution exists is good enough for me.

Thank you very much for jumping in and fixing this.

comment:9 Changed 11 months ago by ezyang

  • Resolution set to fixed
  • Status changed from merge to closed

OK, closing this then.

Note: See TracTickets for help on using tickets.