Re: hipe_mfait_lock

Louis-Philippe Gauthier writes:
 > Hi Mikael,
 > I got a bit excited and tried it right away. Seems to fix my contention
 > issue on hipe_mfait_lock.
 > 
 > Here's the new lock count output (hipe_mfait_lock is not there!):
 > 
 > https://gist.github.com/lpgauth/e85efa59b96726af235d
 > 
 > Thanks,
 > LP

Thanks for confirming that it had the intended effect.
I've sent OTP a pull request for it.

/Mikael

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

-- 
_______________________________________________
erlang-bugs mailing list
[email protected]
http://erlang.org/mailman/listinfo/erlang-bugs