<div dir="ltr">Hi Mikael,<div>I got a bit excited and tried it right away. Seems to fix my contention issue on <span style="font-size:12.8000001907349px">hipe_mfait_lock.</span></div><div><br></div><div>Here's the new lock count output (<span style="font-size:12.8000001907349px">hipe_mfait_lock is not there!):</span></div><div><span style="font-size:12.8000001907349px"><br></span></div><div><a href="https://gist.github.com/lpgauth/e85efa59b96726af235d">https://gist.github.com/lpgauth/e85efa59b96726af235d</a><br></div><div><br></div><div>Thanks,</div><div>LP</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 23, 2015 at 9:28 AM, Louis-Philippe Gauthier <span dir="ltr"><<a href="mailto:louis-philippe.gauthier@adgear.com" target="_blank">louis-philippe.gauthier@adgear.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Mikael,<div>Awesome work, I'll give the patch a try this week.</div><div><br></div><div>Thanks,</div><div>LP</div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Mon, Feb 23, 2015 at 6:30 AM, Mikael Pettersson <span dir="ltr"><<a href="mailto:mikpelinux@gmail.com" target="_blank">mikpelinux@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div><div>Louis-Philippe Gauthier writes:<br>
 > Hi Sverker,<br>
 > That's not a bad idea, I'll try that approach!<br>
 ><br>
 > Thanks,<br>
 > LP<br>
 ><br>
 > On Thu, Jan 15, 2015 at 12:27 AM, Sverker Eriksson <<br>
 > <a href="mailto:sverker.eriksson@ericsson.com" target="_blank">sverker.eriksson@ericsson.com</a>> wrote:<br>
 ><br>
 > >  One quite simple improvement is to change the hipe_mfait_lock to be a<br>
 > > read-write mutex (erts_smp_rwmtx_t).<br>
 > > And then do read-lock at lookup and exclusive write-lock only when<br>
 > > inserting a new entry in the table.<br>
 > ><br>
 > > I don't think that requires much knowledge about the VM to pull off ;-) .<br>
 > ><br>
 > > /Sverker, Erlang/OTP<br>
 > ><br>
 > ><br>
 > ><br>
 > ><br>
 > > On 01/14/2015 12:34 AM, Louis-Philippe Gauthier wrote:<br>
 > ><br>
 > > Hi erlang-bugs,<br>
 > > I couple of months ago I tried running our full application using HiPE. I<br>
 > > ran into several issues, some manageable, some not... The most problematic<br>
 > > issue was due to locking, specifically, the hipe_mfait_lock (you can see<br>
 > > the lock counter output in the gist bellow).<br>
 > > <a href="https://gist.github.com/lpgauth/2b3220f4bceeed6f62d0" target="_blank">https://gist.github.com/lpgauth/2b3220f4bceeed6f62d0</a><br>
 > ><br>
 > > Looking at the code it's obvious that this is a known problem... the<br>
 > > following comment was added when the lock was added in 2010.<br>
 > ><br>
 > > "XXX: Redesign apply et al to avoid those updates."<a href="https://github.com/erlang/otp/blob/maint/erts/emulator/hipe/hipe_bif0.c#L1218" target="_blank">https://github.com/erlang/otp/blob/maint/erts/emulator/hipe/hipe_bif0.c#L1218</a><br>
 > ><br>
 > > Unfortunately, I'm don't know the runtime enough to start patching it... so<br>
 > > instead I'm reporting it.<br>
<br>
</div></div>Please try the attached patch (against 17.4, should apply to older<br>
versions too by I haven't checked), and let me know if it makes a<br>
noticeable improvement to your use case.<br>
<br>
Run-time apply:s now do their lookups using a shared read-lock, and<br>
only if their lookups fail do they re-lock with an exclusive write-lock<br>
in order to insert new data.  Module loading functions always take<br>
exclusive locks, which matches the current behaviour.<br>
<span><font color="#888888"><br>
/Mikael<br>
<br>
</font></span><br></div></div>--- otp_src_17.4/erts/emulator/hipe/hipe_bif0.c.~1~     2014-12-09 21:11:07.000000000 +0100<br>
+++ otp_src_17.4/erts/emulator/hipe/hipe_bif0.c 2015-02-22 21:32:55.361884741 +0100<br>
@@ -1217,22 +1217,32 @@ static struct {<br>
      * they create a new stub for the mfa, which forces locking.<br>
      * XXX: Redesign apply et al to avoid those updates.<br>
      */<br>
-    erts_smp_mtx_t lock;<br>
+    erts_smp_rwmtx_t lock;<br>
 } hipe_mfa_info_table;<br>
<br>
 static inline void hipe_mfa_info_table_init_lock(void)<br>
 {<br>
-    erts_smp_mtx_init(&hipe_mfa_info_table.lock, "hipe_mfait_lock");<br>
+    erts_smp_rwmtx_init(&hipe_mfa_info_table.lock, "hipe_mfait_lock");<br>
 }<br>
<br>
-static inline void hipe_mfa_info_table_lock(void)<br>
+static inline void hipe_mfa_info_table_rlock(void)<br>
 {<br>
-    erts_smp_mtx_lock(&hipe_mfa_info_table.lock);<br>
+    erts_smp_rwmtx_rlock(&hipe_mfa_info_table.lock);<br>
 }<br>
<br>
-static inline void hipe_mfa_info_table_unlock(void)<br>
+static inline void hipe_mfa_info_table_runlock(void)<br>
 {<br>
-    erts_smp_mtx_unlock(&hipe_mfa_info_table.lock);<br>
+    erts_smp_rwmtx_runlock(&hipe_mfa_info_table.lock);<br>
+}<br>
+<br>
+static inline void hipe_mfa_info_table_rwlock(void)<br>
+{<br>
+    erts_smp_rwmtx_rwlock(&hipe_mfa_info_table.lock);<br>
+}<br>
+<br>
+static inline void hipe_mfa_info_table_rwunlock(void)<br>
+{<br>
+    erts_smp_rwmtx_rwunlock(&hipe_mfa_info_table.lock);<br>
 }<br>
<br>
 #define HIPE_MFA_HASH(M,F,A)   ((M) * (F) + (A))<br>
@@ -1333,6 +1343,7 @@ void *hipe_mfa_find_na(Eterm m, Eterm f,<br>
 }<br>
 #endif<br>
<br>
+/* PRE: called with write lock held */<br>
 static struct hipe_mfa_info *hipe_mfa_info_table_put_locked(Eterm m, Eterm f, unsigned int arity)<br>
 {<br>
     unsigned long h;<br>
@@ -1362,7 +1373,7 @@ static void hipe_mfa_set_na(Eterm m, Ete<br>
 {<br>
     struct hipe_mfa_info *p;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     p = hipe_mfa_info_table_put_locked(m, f, arity);<br>
 #ifdef DEBUG_LINKER<br>
     printf("%s: ", __FUNCTION__);<br>
@@ -1372,7 +1383,7 @@ static void hipe_mfa_set_na(Eterm m, Ete<br>
     p->local_address = address;<br>
     if (is_exported)<br>
        p->remote_address = address;<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
 }<br>
<br>
 #if defined(__powerpc__) || defined(__ppc__) || defined(__powerpc64__) || defined(__arm__)<br>
@@ -1381,10 +1392,10 @@ void *hipe_mfa_get_trampoline(Eterm m, E<br>
     struct hipe_mfa_info *p;<br>
     void *trampoline;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
-    p = hipe_mfa_info_table_put_locked(m, f, arity);<br>
-    trampoline = p->trampoline;<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rlock();<br>
+    p = hipe_mfa_info_table_get_locked(m, f, arity);<br>
+    trampoline = p ? p->trampoline : NULL;<br>
+    hipe_mfa_info_table_runlock();<br>
     return trampoline;<br>
 }<br>
<br>
@@ -1392,10 +1403,10 @@ void hipe_mfa_set_trampoline(Eterm m, Et<br>
 {<br>
     struct hipe_mfa_info *p;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     p = hipe_mfa_info_table_put_locked(m, f, arity);<br>
     p->trampoline = trampoline;<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
 }<br>
 #endif<br>
<br>
@@ -1426,7 +1437,7 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo<br>
     struct mfa mfa;<br>
     struct hipe_mfa_info *p;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     lst = BIF_ARG_1;<br>
     while (is_list(lst)) {<br>
        if (!term_to_mfa(CAR(list_val(lst)), &mfa))<br>
@@ -1455,7 +1466,7 @@ BIF_RETTYPE hipe_bifs_invalidate_funinfo<br>
            }<br>
        }<br>
     }<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
     if (is_not_nil(lst))<br>
        BIF_ERROR(BIF_P, BADARG);<br>
     BIF_RET(NIL);<br>
@@ -1469,7 +1480,7 @@ void hipe_mfa_save_orig_beam_op(Eterm mo<br>
     orig_beam_op = pc[0];<br>
     if (orig_beam_op != BeamOpCode(op_hipe_trap_call_closure) &&<br>
        orig_beam_op != BeamOpCode(op_hipe_trap_call)) {<br>
-       hipe_mfa_info_table_lock();<br>
+       hipe_mfa_info_table_rwlock();<br>
        p = hipe_mfa_info_table_put_locked(mod, fun, ari);<br>
 #ifdef DEBUG_LINKER<br>
        printf("%s: ", __FUNCTION__);<br>
@@ -1478,7 +1489,7 @@ void hipe_mfa_save_orig_beam_op(Eterm mo<br>
 #endif<br>
        p->beam_code = pc;<br>
        p->orig_beam_op = orig_beam_op;<br>
-       hipe_mfa_info_table_unlock();<br>
+       hipe_mfa_info_table_rwunlock();<br>
     } else {<br>
 #ifdef DEBUG_LINKER<br>
        printf("%s: ", __FUNCTION__);<br>
@@ -1505,7 +1516,8 @@ static void *hipe_make_stub(Eterm m, Ete<br>
     return StubAddress;<br>
 }<br>
<br>
-static void *hipe_get_na_nofail_locked(Eterm m, Eterm f, unsigned int a, int is_remote)<br>
+/* PRE: called with read or write lock held */<br>
+static void *hipe_get_na_try_locked(Eterm m, Eterm f, unsigned int a, int is_remote, struct hipe_mfa_info **pp)<br>
 {<br>
     struct hipe_mfa_info *p;<br>
     void *address;<br>
@@ -1523,7 +1535,20 @@ static void *hipe_get_na_nofail_locked(E<br>
        address = p->remote_address;<br>
        if (address)<br>
            return address;<br>
-    } else<br>
+    }<br>
+    /* Caller must take the slow path with the write lock held, but allow<br>
+       it to avoid some work if it already holds the write lock.  */<br>
+    if (pp)<br>
+       *pp = p;<br>
+    return NULL;<br>
+}<br>
+<br>
+/* PRE: called with write lock held */<br>
+static void *hipe_get_na_slow_locked(Eterm m, Eterm f, unsigned int a, int is_remote, struct hipe_mfa_info *p)<br>
+{<br>
+    void *address;<br>
+<br>
+    if (!p)<br>
        p = hipe_mfa_info_table_put_locked(m, f, a);<br>
     address = hipe_make_stub(m, f, a, is_remote);<br>
     /* XXX: how to tell if a BEAM MFA is exported or not? */<br>
@@ -1531,14 +1556,34 @@ static void *hipe_get_na_nofail_locked(E<br>
     return address;<br>
 }<br>
<br>
+/* PRE: called with write lock held */<br>
+static void *hipe_get_na_nofail_locked(Eterm m, Eterm f, unsigned int a, int is_remote)<br>
+{<br>
+    struct hipe_mfa_info *p /*= NULL*/;<br>
+    void *address;<br>
+<br>
+    address = hipe_get_na_try_locked(m, f, a, is_remote, &p);<br>
+    if (address)<br>
+       return address;<br>
+<br>
+    address = hipe_get_na_slow_locked(m, f, a, is_remote, p);<br>
+    return address;<br>
+}<br>
+<br>
 static void *hipe_get_na_nofail(Eterm m, Eterm f, unsigned int a, int is_remote)<br>
 {<br>
-    void *p;<br>
+    void *address;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
-    p = hipe_get_na_nofail_locked(m, f, a, is_remote);<br>
-    hipe_mfa_info_table_unlock();<br>
-    return p;<br>
+    hipe_mfa_info_table_rlock();<br>
+    address = hipe_get_na_try_locked(m, f, a, is_remote, NULL);<br>
+    hipe_mfa_info_table_runlock();<br>
+    if (address)<br>
+       return address;<br>
+<br>
+    hipe_mfa_info_table_rwlock();<br>
+    address = hipe_get_na_slow_locked(m, f, a, is_remote, NULL);<br>
+    hipe_mfa_info_table_rwunlock();<br>
+    return address;<br>
 }<br>
<br>
 /* used for apply/3 in hipe_mode_switch */<br>
@@ -1617,7 +1662,7 @@ int hipe_find_mfa_from_ra(const void *ra<br>
     /* Note about locking: the table is only updated from the<br>
        loader, which runs with the rest of the system suspended. */<br>
     /* XXX: alas not true; see comment at hipe_mfa_info_table.lock */<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     bucket = hipe_mfa_info_table.bucket;<br>
     nrbuckets = 1 << hipe_mfa_info_table.log2size;<br>
     mfa = NULL;<br>
@@ -1638,7 +1683,7 @@ int hipe_find_mfa_from_ra(const void *ra<br>
        *f = mfa->f;<br>
        *a = mfa->a;<br>
     }<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
     return mfa ? 1 : 0;<br>
 }<br>
<br>
@@ -1715,7 +1760,7 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIS<br>
       default:<br>
        goto badarg;<br>
     }<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     callee_mfa = hipe_mfa_info_table_put_locked(callee.mod, callee.fun, callee.ari);<br>
     caller_mfa = hipe_mfa_info_table_put_locked(caller.mod, caller.fun, caller.ari);<br>
<br>
@@ -1731,7 +1776,7 @@ BIF_RETTYPE hipe_bifs_add_ref_2(BIF_ALIS<br>
     ref->flags = flags;<br>
     ref->next = callee_mfa->referred_from;<br>
     callee_mfa->referred_from = ref;<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
<br>
     BIF_RET(NIL);<br>
<br>
@@ -1751,12 +1796,12 @@ BIF_RETTYPE hipe_bifs_mark_referred_from<br>
<br>
     if (!term_to_mfa(BIF_ARG_1, &mfa))<br>
        BIF_ERROR(BIF_P, BADARG);<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);<br>
     if (p)<br>
        for (ref = p->referred_from; ref != NULL; ref = ref->next)<br>
            ref->flags |= REF_FLAG_PENDING_REDIRECT;<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
     BIF_RET(NIL);<br>
 }<br>
<br>
@@ -1770,7 +1815,7 @@ static void hipe_purge_all_refs(void)<br>
     struct hipe_mfa_info **bucket;<br>
     unsigned int i, nrbuckets;<br>
<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
<br>
     bucket = hipe_mfa_info_table.bucket;<br>
     nrbuckets = 1 << hipe_mfa_info_table.log2size;<br>
@@ -1792,7 +1837,7 @@ static void hipe_purge_all_refs(void)<br>
            erts_free(ERTS_ALC_T_HIPE, mfa);<br>
        }<br>
     }<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
 }<br>
<br>
 BIF_RETTYPE hipe_bifs_remove_refs_from_1(BIF_ALIST_1)<br>
@@ -1809,7 +1854,7 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1<br>
<br>
     if (!term_to_mfa(BIF_ARG_1, &mfa))<br>
        BIF_ERROR(BIF_P, BADARG);<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     caller_mfa = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);<br>
     if (caller_mfa) {<br>
        refers_to = caller_mfa->refers_to;<br>
@@ -1840,7 +1885,7 @@ BIF_RETTYPE hipe_bifs_remove_refs_from_1<br>
        }<br>
        caller_mfa->refers_to = NULL;<br>
     }<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
     BIF_RET(am_ok);<br>
 }<br>
<br>
@@ -1859,7 +1904,7 @@ BIF_RETTYPE hipe_bifs_redirect_referred_<br>
<br>
     if (!term_to_mfa(BIF_ARG_1, &mfa))<br>
        BIF_ERROR(BIF_P, BADARG);<br>
-    hipe_mfa_info_table_lock();<br>
+    hipe_mfa_info_table_rwlock();<br>
     p = hipe_mfa_info_table_get_locked(mfa.mod, mfa.fun, mfa.ari);<br>
     if (p) {<br>
        prev = &p->referred_from;<br>
@@ -1890,7 +1935,7 @@ BIF_RETTYPE hipe_bifs_redirect_referred_<br>
            }<br>
        }<br>
     }<br>
-    hipe_mfa_info_table_unlock();<br>
+    hipe_mfa_info_table_rwunlock();<br>
     BIF_RET(NIL);<br>
 }<br>
<br>
<br></blockquote></div><br></div>
</blockquote></div><br></div>