This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patchv3] Fix 100x slowdown regression on DWZ files
- From: Doug Evans <dje at google dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 22 Jan 2015 10:45:08 -0800
- Subject: Re: [patchv3] Fix 100x slowdown regression on DWZ files
- Authentication-results: sourceware.org; auth=none
- References: <CADPb22Q2r9Vnne5rDapAy=AuD2AKBU01TRANw3djo5Xk1icvNw at mail dot gmail dot com> <21548 dot 37770 dot 274873 dot 760290 at ruffy2 dot mtv dot corp dot google dot com> <20141002155653 dot GA9001 at host2 dot jankratochvil dot net> <20141231192335 dot GA8188 at host2 dot jankratochvil dot net> <21677 dot 57646 dot 178793 dot 836948 at ruffy2 dot mtv dot corp dot google dot com> <20150114202618 dot GA21056 at host2 dot jankratochvil dot net>
Jan Kratochvil writes:
> Hi Doug,
>
> On Thu, 08 Jan 2015 02:45:18 +0100, Doug Evans wrote:
> > + line_header_local.offset.sect_off = line_offset;
> > + line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
> > + slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
> > + &line_header_local, NO_INSERT);
> >
> > Do hashtables support calling htab_find_slot with INSERT but then
> > not assigning the slot a value if it was empty?
> > I could be wrong but I think it does.
> > If so, we can remove one call to htab_find_slot here.
>
> If dwarf_decode_line_header() fails we have nothing to put there. If we had
> done INSERT it is a problem as discussed in previous mail.
>
>
> > + /* For DW_TAG_compile_unit we need info like symtab::linetable which
> > + is not present in *SLOT. */
> > + if (die->tag == DW_TAG_partial_unit && slot != NULL)
> > + {
> > + gdb_assert (*slot != NULL);
> > + cu->line_header = *slot;
> > + return;
> > + }
> > +
> > + line_header = dwarf_decode_line_header (line_offset, cu);
> > + if (line_header == NULL)
> > + return;
> > + cu->line_header = line_header;
> > +
> > + slot = htab_find_slot (dwarf2_per_objfile->line_header_hash,
> > + &line_header_local, INSERT);
> > + gdb_assert (slot != NULL);
> > + if (*slot == NULL)
> > + *slot = line_header;
> > + else
> > + {
> > + gdb_assert (die->tag != DW_TAG_partial_unit);
> > + make_cleanup (free_cu_line_header, cu);
> > }
> > + decode_mapping = (die->tag != DW_TAG_partial_unit);
> > + dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc,
> > + decode_mapping);
> >
> > This is a bit confusing to follow.
> > If the slot was empty we save line_header in it (and don't record a cleanup),
> > but if wasn't empty we don't update *slot but do record a cleanup.
> > Presumably there's a reason, but it's hard to follow.
>
> I do not see how to make it differently. I have put there more comments.
>
>
> > It would be simpler to just free any previous entry and conditionally
> > update *slot. Is there a reason to not do that?
> > Can you add a clarifying comment?
>
> We cannot - comment with the reason is now in the code.
>
>
> > Plus I'm worried about increased memory usage in the non-dwz case.
> > IIUC, the non-dwz case will always have *slot == NULL, and thus we'll
> > always be saving line header entries we'll never need again.
>
> <rant>Those are few bytes for each expanded CU. The problem of GDB is that it
> needlessly expands thousands of CUs. Saving each byte of a decoded CU is not
> the right way how to fix the excessive memory consumption.</rant>
>
> But added there for it 'dwarf2_per_objfile->seen_partial_unit'.
>
>
> > Also, it looks like line_header_hash (and its entries) aren't
> > deleted when the objfile goes away. Missing call to htab_delete.
>
> Fixed.
>
>
> > A few comments inline.
>
> BTW I would prefer s/^/> / for the patch, the comments are difficult to find
> this way.
>
>
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora22pre-linux-gnu in default
> mode, other modes still run but hopefully they will be OK.
>
>
> Thanks,
> Jan
> 2015-01-14 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> Fix 100x slowdown regression on DWZ files.
> * dwarf2read.c (struct dwarf2_per_objfile): Add seen_partial_unit and
> line_header_hash.
> (struct line_header): Add offset and offset_in_dwz.
> (dwarf_decode_lines): Add parameter decode_mapping to the declaration.
> (free_line_header_voidp): New declaration.
> (line_header_hash, line_header_hash_voidp, line_header_eq_voidp): New
> functions.
> (dwarf2_build_include_psymtabs): Update dwarf_decode_lines caller.
> (handle_DW_AT_stmt_list): Use seen_partial_unit and line_header_hash.
> (free_line_header_voidp): New function.
> (dwarf_decode_line_header): Initialize offset and offset_in_dwz.
> (dwarf_decode_lines): New parameter decode_mapping, use it.
> (dwarf2_free_objfile): Free line_header_hash.
Hi.
Thanks, I understand things now.
I tweaked the patch a bit so that a year from now I'll still understand it. :-)
Appended is a diff to your patch, and then the full modified patch.
In my mind it's easier to just treat a non-NULL value for line_header_hash
as the flag to decide whether we're using the hash (instead of
seen_partial_unit).
Sound ok?
-- patch of the patch ---
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3d27d70..e7246f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -285,9 +285,6 @@ struct dwarf2_per_objfile
or we are faking it for OBJF_READNOW's sake. */
unsigned char using_index;
- /* True if we have already seen a DW_TAG_partial_unit. */
- unsigned char seen_partial_unit;
-
/* The mapped index, or NULL if .gdb_index is missing or not being used. */
struct mapped_index *index_table;
@@ -9056,11 +9053,14 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
line_offset = DW_UNSND (attr);
- if (die->tag == DW_TAG_partial_unit)
- dwarf2_per_objfile->seen_partial_unit = 1;
+ /* The line header hash table is only created if needed (it exists to
+ prevent redundant reading of the line table for partial_units).
+ If we're given a partial_unit, we'll need it. If we're given a
+ compile_unit, then use the line header hash table if it's already
+ created, but don't create one just yet. */
if (dwarf2_per_objfile->line_header_hash == NULL
- && dwarf2_per_objfile->seen_partial_unit)
+ && die->tag == DW_TAG_partial_unit)
{
dwarf2_per_objfile->line_header_hash
= htab_create_alloc_ex (127, line_header_hash_voidp,
@@ -9074,14 +9074,15 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
line_header_local.offset.sect_off = line_offset;
line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
line_header_local_hash = line_header_hash (&line_header_local);
- if (dwarf2_per_objfile->seen_partial_unit)
+ if (dwarf2_per_objfile->line_header_hash != NULL)
{
slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
&line_header_local,
line_header_local_hash, NO_INSERT);
/* For DW_TAG_compile_unit we need info like symtab::linetable which
- is not present in *SLOT. */
+ is not present in *SLOT (since if there is something in *SLOT then
+ it will be for a partial_unit). */
if (die->tag == DW_TAG_partial_unit && slot != NULL)
{
gdb_assert (*slot != NULL);
@@ -9096,7 +9097,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
if (cu->line_header == NULL)
return;
- if (!dwarf2_per_objfile->seen_partial_unit)
+ if (dwarf2_per_objfile->line_header_hash == NULL)
slot = NULL;
else
{
@@ -9113,10 +9114,11 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
}
else
{
- /* We cannot free_line_header (*slot) as that struct line_header
- may be already used by multiple CUs. Create only temporary
- decoded line_header for this CU - it may happen at most once
- for each line number information unit. */
+ /* We cannot free any current entry in (*slot) as that struct line_header
+ may be already used by multiple CUs. Create only temporary decoded
+ line_header for this CU - it may happen at most once for each line
+ number information unit. And if we're not using line_header_hash
+ then this is what we want as well. */
gdb_assert (die->tag != DW_TAG_partial_unit);
make_cleanup (free_cu_line_header, cu);
}
--- full patch ---
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 86c3a73..e7246f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -308,6 +308,9 @@ struct dwarf2_per_objfile
/* The CUs we recently read. */
VEC (dwarf2_per_cu_ptr) *just_read_cus;
+
+ /* Table containing line_header indexed by offset and offset_in_dwz. */
+ htab_t line_header_hash;
};
static struct dwarf2_per_objfile *dwarf2_per_objfile;
@@ -1024,6 +1027,12 @@ typedef void (die_reader_func_ftype) (const struct die_reader_specs *reader,
which contains the following information. */
struct line_header
{
+ /* Offset of line number information in .debug_line section. */
+ sect_offset offset;
+
+ /* OFFSET is for struct dwz_file associated with dwarf2_per_objfile. */
+ unsigned offset_in_dwz : 1;
+
unsigned int total_length;
unsigned short version;
unsigned int header_length;
@@ -1512,7 +1521,7 @@ static struct line_header *dwarf_decode_line_header (unsigned int offset,
static void dwarf_decode_lines (struct line_header *, const char *,
struct dwarf2_cu *, struct partial_symtab *,
- CORE_ADDR);
+ CORE_ADDR, int decode_mapping);
static void dwarf2_start_subfile (const char *, const char *);
@@ -1844,6 +1853,8 @@ static void free_dwo_file_cleanup (void *);
static void process_cu_includes (void);
static void check_producer (struct dwarf2_cu *cu);
+
+static void free_line_header_voidp (void *arg);
/* Various complaints about symbol reading that don't abort the process. */
@@ -1910,6 +1921,37 @@ dwarf2_invalid_attrib_class_complaint (const char *arg1, const char *arg2)
_("invalid attribute class or form for '%s' in '%s'"),
arg1, arg2);
}
+
+/* Hash function for line_header_hash. */
+
+static hashval_t
+line_header_hash (const struct line_header *ofs)
+{
+ return ofs->offset.sect_off ^ ofs->offset_in_dwz;
+}
+
+/* Hash function for htab_create_alloc_ex for line_header_hash. */
+
+static hashval_t
+line_header_hash_voidp (const void *item)
+{
+ const struct line_header *ofs = item;
+
+ return line_header_hash (ofs);
+}
+
+/* Equality function for line_header_hash. */
+
+static int
+line_header_eq_voidp (const void *item_lhs, const void *item_rhs)
+{
+ const struct line_header *ofs_lhs = item_lhs;
+ const struct line_header *ofs_rhs = item_rhs;
+
+ return (ofs_lhs->offset.sect_off == ofs_rhs->offset.sect_off
+ && ofs_lhs->offset_in_dwz == ofs_rhs->offset_in_dwz);
+}
+
#if WORDS_BIGENDIAN
@@ -4452,7 +4494,7 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
return; /* No linetable, so no includes. */
/* NOTE: pst->dirname is DW_AT_comp_dir (if present). */
- dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow);
+ dwarf_decode_lines (lh, pst->dirname, cu, pst, pst->textlow, 1);
free_line_header (lh);
}
@@ -8994,24 +9036,95 @@ static void
handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
const char *comp_dir, CORE_ADDR lowpc) /* ARI: editCase function */
{
+ struct objfile *objfile = dwarf2_per_objfile->objfile;
struct attribute *attr;
+ unsigned int line_offset;
+ struct line_header line_header_local;
+ hashval_t line_header_local_hash;
+ unsigned u;
+ void **slot;
+ int decode_mapping;
gdb_assert (! cu->per_cu->is_debug_types);
attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
- if (attr)
+ if (attr == NULL)
+ return;
+
+ line_offset = DW_UNSND (attr);
+
+ /* The line header hash table is only created if needed (it exists to
+ prevent redundant reading of the line table for partial_units).
+ If we're given a partial_unit, we'll need it. If we're given a
+ compile_unit, then use the line header hash table if it's already
+ created, but don't create one just yet. */
+
+ if (dwarf2_per_objfile->line_header_hash == NULL
+ && die->tag == DW_TAG_partial_unit)
+ {
+ dwarf2_per_objfile->line_header_hash
+ = htab_create_alloc_ex (127, line_header_hash_voidp,
+ line_header_eq_voidp,
+ free_line_header_voidp,
+ &objfile->objfile_obstack,
+ hashtab_obstack_allocate,
+ dummy_obstack_deallocate);
+ }
+
+ line_header_local.offset.sect_off = line_offset;
+ line_header_local.offset_in_dwz = cu->per_cu->is_dwz;
+ line_header_local_hash = line_header_hash (&line_header_local);
+ if (dwarf2_per_objfile->line_header_hash != NULL)
{
- unsigned int line_offset = DW_UNSND (attr);
- struct line_header *line_header
- = dwarf_decode_line_header (line_offset, cu);
+ slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+ &line_header_local,
+ line_header_local_hash, NO_INSERT);
- if (line_header)
+ /* For DW_TAG_compile_unit we need info like symtab::linetable which
+ is not present in *SLOT (since if there is something in *SLOT then
+ it will be for a partial_unit). */
+ if (die->tag == DW_TAG_partial_unit && slot != NULL)
{
- cu->line_header = line_header;
- make_cleanup (free_cu_line_header, cu);
- dwarf_decode_lines (line_header, comp_dir, cu, NULL, lowpc);
+ gdb_assert (*slot != NULL);
+ cu->line_header = *slot;
+ return;
}
}
+
+ /* dwarf_decode_line_header does not yet provide sufficient information.
+ We always have to call also dwarf_decode_lines for it. */
+ cu->line_header = dwarf_decode_line_header (line_offset, cu);
+ if (cu->line_header == NULL)
+ return;
+
+ if (dwarf2_per_objfile->line_header_hash == NULL)
+ slot = NULL;
+ else
+ {
+ slot = htab_find_slot_with_hash (dwarf2_per_objfile->line_header_hash,
+ &line_header_local,
+ line_header_local_hash, INSERT);
+ gdb_assert (slot != NULL);
+ }
+ if (slot != NULL && *slot == NULL)
+ {
+ /* This newly decoded line number information unit will be owned
+ by line_header_hash hash table. */
+ *slot = cu->line_header;
+ }
+ else
+ {
+ /* We cannot free any current entry in (*slot) as that struct line_header
+ may be already used by multiple CUs. Create only temporary decoded
+ line_header for this CU - it may happen at most once for each line
+ number information unit. And if we're not using line_header_hash
+ then this is what we want as well. */
+ gdb_assert (die->tag != DW_TAG_partial_unit);
+ make_cleanup (free_cu_line_header, cu);
+ }
+ decode_mapping = (die->tag != DW_TAG_partial_unit);
+ dwarf_decode_lines (cu->line_header, comp_dir, cu, NULL, lowpc,
+ decode_mapping);
}
/* Process DW_TAG_compile_unit or DW_TAG_partial_unit. */
@@ -16908,6 +17021,16 @@ free_line_header (struct line_header *lh)
xfree (lh);
}
+/* Stub for free_line_header to match void * callback types. */
+
+static void
+free_line_header_voidp (void *arg)
+{
+ struct line_header *lh = arg;
+
+ free_line_header (lh);
+}
+
/* Add an entry to LH's include directory table. */
static void
@@ -17038,6 +17161,9 @@ dwarf_decode_line_header (unsigned int offset, struct dwarf2_cu *cu)
back_to = make_cleanup ((make_cleanup_ftype *) free_line_header,
(void *) lh);
+ lh->offset.sect_off = offset;
+ lh->offset_in_dwz = cu->per_cu->is_dwz;
+
line_ptr = section->buffer + offset;
/* Read in the header. */
@@ -17665,17 +17791,22 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
E.g. expand_line_sal requires this when finding psymtabs to expand.
A good testcase for this is mb-inline.exp.
- LOWPC is the lowest address in CU (or 0 if not known). */
+ LOWPC is the lowest address in CU (or 0 if not known).
+
+ Boolean DECODE_MAPPING specifies we need to fully decode .debug_line
+ for its PC<->lines mapping information. Otherwise only the filename
+ table is read in. */
static void
dwarf_decode_lines (struct line_header *lh, const char *comp_dir,
struct dwarf2_cu *cu, struct partial_symtab *pst,
- CORE_ADDR lowpc)
+ CORE_ADDR lowpc, int decode_mapping)
{
struct objfile *objfile = cu->objfile;
const int decode_for_pst_p = (pst != NULL);
- dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
+ if (decode_mapping)
+ dwarf_decode_lines_1 (lh, cu, decode_for_pst_p, lowpc);
if (decode_for_pst_p)
{
@@ -21772,6 +21903,9 @@ dwarf2_free_objfile (struct objfile *objfile)
if (dwarf2_per_objfile->quick_file_names_table)
htab_delete (dwarf2_per_objfile->quick_file_names_table);
+ if (dwarf2_per_objfile->line_header_hash)
+ htab_delete (dwarf2_per_objfile->line_header_hash);
+
/* Everything else should be on the objfile obstack. */
}