Re: [patchv3] Fix 100x slowdown regression on DWZ files

[Doug Evans <dje at google dot com>]

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.  */
 }