Hi Aaron, On Sun, 2025-06-22 at 20:00 -0400, Aaron Merey wrote: > Improve __libdw_dieabbrev performance by removing abbrev_lock. This > lock protects the Dwarf_Die abbrev member due to lazy loading. > > Instead, eagerly load abbrev during Dwarf_Die initialization so that > the member is readonly and requires no locking.
Could you explain a bit more how this works? As far as I understand you made the following changes: - Made the CUDIE and SUBDIE macros into static inline functions. - This made it so that the usage in dwarf_decl_file and getfiles needed to introduce a temporary Dwarf_Die variable (fine). - The new functions return a Dwarf_Die which have all fields initialized, not just .cu and .addr. - Specifically they call dwarf_tag on the to be returned Dwarf_Die. This seems to be done to trigger a call to __libdw_dieabbrev (die, NULL). If so, why not do that directly? - __libdw_dieabbrev has the abbrev_lock removed. And no longer sets die->abbrev on failure (why?) - __libdw_dieabbrev now calls __libdw_findabbrev (die->cu, code) without any lock. - __libdw_findabbrev calls Dwarf_Abbrev_Hash_find on cu->abbrev_hash, which should be fine, since it is a concurrent hash table. - But then if not found it checks cu->last_abbrev_offset which I don't understand how that is fine without any locking. It is later modified in this function. And it seems there can be concurrent calls for the same cu? - __libdw_findabbrev then calls __libdw_getabbrev which uses Dwarf_Abbrev_Hash_find and Dwarf_Abbrev_Hash_insert on the cu->abbrev_hash which should be fine because that is the concurrent hash table also used above. So I think this is almost, but not entirely thread-safe, with the locking. We need some solution for the concurrent last_abbrev_offset manipulation. > Signed-off-by: Aaron Merey <ame...@redhat.com> > --- > libdw/dwarf_decl_file.c | 3 +- > libdw/dwarf_end.c | 1 - > libdw/dwarf_getscopevar.c | 3 +- > libdw/libdwP.h | 65 +++++++++++++++++---------------------- > libdw/libdw_findcu.c | 1 - > 5 files changed, 32 insertions(+), 41 deletions(-) > > diff --git a/libdw/dwarf_decl_file.c b/libdw/dwarf_decl_file.c > index 1b91a4e9..03040e12 100644 > --- a/libdw/dwarf_decl_file.c > +++ b/libdw/dwarf_decl_file.c > @@ -50,7 +50,8 @@ dwarf_decl_file (Dwarf_Die *die) > > Dwarf_Files *files; > size_t nfiles; > - if (INTUSE(dwarf_getsrcfiles) (&CUDIE (attr_mem.cu), &files, &nfiles) != 0) > + Dwarf_Die cudie = CUDIE (attr_mem.cu); > + if (INTUSE(dwarf_getsrcfiles) (&cudie, &files, &nfiles) != 0) > return NULL; > > if (idx >= nfiles) > diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c > index 1628e448..51021d64 100644 > --- a/libdw/dwarf_end.c > +++ b/libdw/dwarf_end.c > @@ -68,7 +68,6 @@ cu_free (void *arg) > && p != p->dbg->fake_addr_cu) > { > Dwarf_Abbrev_Hash_free (&p->abbrev_hash); > - rwlock_fini (p->abbrev_lock); > rwlock_fini (p->split_lock); > mutex_fini (p->src_lock); > mutex_fini (p->str_off_base_lock); > diff --git a/libdw/dwarf_getscopevar.c b/libdw/dwarf_getscopevar.c > index 7b1416f3..306ccae0 100644 > --- a/libdw/dwarf_getscopevar.c > +++ b/libdw/dwarf_getscopevar.c > @@ -40,7 +40,8 @@ > static int > getfiles (Dwarf_Die *die, Dwarf_Files **files) > { > - return INTUSE(dwarf_getsrcfiles) (&CUDIE (die->cu), files, NULL); > + Dwarf_Die cudie = CUDIE (die->cu); > + return INTUSE(dwarf_getsrcfiles) (&cudie, files, NULL); > } > > /* Fetch an attribute that should have a constant integer form. */ > diff --git a/libdw/libdwP.h b/libdw/libdwP.h > index de80cd4e..4a528c9c 100644 > --- a/libdw/libdwP.h > +++ b/libdw/libdwP.h > @@ -455,10 +455,6 @@ struct Dwarf_CU > Don't access directly, call __libdw_cu_locs_base. */ > Dwarf_Off locs_base; > > - /* Synchronize access to the abbrev member of a Dwarf_Die that > - refers to this Dwarf_CU. Covers __libdw_die_abbrev. */ > - rwlock_define(, abbrev_lock); > - > /* Synchronize access to the split member of this Dwarf_CU. > Covers __libdw_find_split_unit. */ > rwlock_define(, split_lock); > @@ -595,23 +591,6 @@ __libdw_first_die_off_from_cu (struct Dwarf_CU *cu) > cu->unit_type); > } > > -#define CUDIE(fromcu) > \ > - ((Dwarf_Die) > \ > - { \ > - .cu = (fromcu), \ > - .addr = ((char *) (fromcu)->dbg->sectiondata[cu_sec_idx > (fromcu)]->d_buf \ > - + __libdw_first_die_off_from_cu (fromcu)) \ > - }) > - > -#define SUBDIE(fromcu) > \ > - ((Dwarf_Die) > \ > - { \ > - .cu = (fromcu), \ > - .addr = ((char *) (fromcu)->dbg->sectiondata[cu_sec_idx > (fromcu)]->d_buf \ > - + (fromcu)->start + (fromcu)->subdie_offset) \ > - }) > - > - > /* Prototype of a single .debug_macro operator. */ > typedef struct > { > @@ -820,12 +799,7 @@ __nonnull_attribute__ (1) > __libdw_dieabbrev (Dwarf_Die *die, const unsigned char **readp) > { > if (unlikely (die->cu == NULL)) > - { > - die->abbrev = DWARF_END_ABBREV; > - return DWARF_END_ABBREV; > - } > - > - rwlock_wrlock (die->cu->abbrev_lock); > + return DWARF_END_ABBREV; > > /* Do we need to get the abbreviation, or need to read after the code? */ > if (die->abbrev == NULL || readp != NULL) > @@ -835,25 +809,20 @@ __libdw_dieabbrev (Dwarf_Die *die, const unsigned char > **readp) > const unsigned char *addr = die->addr; > > if (addr >= (const unsigned char *) die->cu->endp) > - { > - die->abbrev = DWARF_END_ABBREV; > - rwlock_unlock (die->cu->abbrev_lock); > - return DWARF_END_ABBREV; > - } > + return DWARF_END_ABBREV; > > get_uleb128 (code, addr, die->cu->endp); > if (readp != NULL) > *readp = addr; > > - /* Find the abbreviation. */ > + /* Find the abbreviation. To improve performance, this write is not > + protected by a lock. It should only be reachable by a single thread > + when initializing the DIE. */ > if (die->abbrev == NULL) > die->abbrev = __libdw_findabbrev (die->cu, code); > } > > - Dwarf_Abbrev *result = die->abbrev; > - rwlock_unlock (die->cu->abbrev_lock); > - > - return result; > + return die->abbrev; > } > > /* Helper functions for form handling. */ > @@ -1112,6 +1081,28 @@ cu_sec_idx (struct Dwarf_CU *cu) > return cu->sec_idx; > } > > +static inline Dwarf_Die > +CUDIE (Dwarf_CU *fromcu) > +{ > + Dwarf_Die res = {0}; > + res.cu = fromcu; > + res.addr = ((char *) (fromcu)->dbg->sectiondata[cu_sec_idx (fromcu)]->d_buf > + + __libdw_first_die_off_from_cu (fromcu)); > + (void) dwarf_tag (&res); /* Set .abbrev */ > + return res; > +} > + > +static inline Dwarf_Die > +SUBDIE (Dwarf_CU *fromcu) > +{ > + Dwarf_Die res = {0}; > + res.cu = fromcu; > + res.addr = ((char *) (fromcu)->dbg->sectiondata[cu_sec_idx (fromcu)]->d_buf > + + (fromcu)->start + (fromcu)->subdie_offset); > + (void) dwarf_tag (&res); /* Set .abbrev */ > + return res; > +} > + > static inline bool > is_cudie (Dwarf_Die *cudie) > { > diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c > index 0e4dcc37..857ebd38 100644 > --- a/libdw/libdw_findcu.c > +++ b/libdw/libdw_findcu.c > @@ -177,7 +177,6 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types) > newp->startp = data->d_buf + newp->start; > newp->endp = data->d_buf + newp->end; > eu_search_tree_init (&newp->locs_tree); > - rwlock_init (newp->abbrev_lock); > rwlock_init (newp->split_lock); > mutex_init (newp->src_lock); > mutex_init (newp->str_off_base_lock);