From patchwork Thu Jan 22 18:45:08 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 4763 Received: (qmail 12991 invoked by alias); 22 Jan 2015 18:46:23 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12869 invoked by uid 89); 22 Jan 2015 18:46:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-qa0-f74.google.com Received: from mail-qa0-f74.google.com (HELO mail-qa0-f74.google.com) (209.85.216.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 22 Jan 2015 18:45:11 +0000 Received: by mail-qa0-f74.google.com with SMTP id x12so200461qac.1 for ; Thu, 22 Jan 2015 10:45:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=7JLpAmJaJc77riNoqdC9gLBcU7Sf6I8Lu1r2th0ZV2k=; b=b+c6GdGmq+JNvjS2UrLeI+jx6JfpcpoTJI2RJL3WJdWppPKGtWbwQicHSwtkiz7ZXl gvgaMTDb1T65CJOUFS4qAojw+CMVSiJpZpUiOeymjEOkOlRGpmjtrKu3ELzMD5iFq5eX 69Wucw2RWxO82tbopa6kG+pdmqYa/8wrVmgqoOMs1JwVfIUl47OdL9OqGm/loLB4+Oeq y8i65A00Ya+2/A4zUSv3L3b+h/lygiNWjvM3bYmNmxFIv9fo8qq6INiGc9cryX/aOrO/ m7GvWuPDvRiFJbqMGqjNUR0AzhiFyzAaQFyo8m1CwmHHEh0V5IxDlhBxe797PS0Z0BMf X8ig== X-Gm-Message-State: ALoCoQnvd7W7y9f+WJm+b/103OF4w3yQ4VWvALRzQT7gClBuH9gmR9K6oISnyD++elkBJYN9y9YNJC2m26kPVahEa4TFFfe9Z8suk+fHl/6L0bsQK3jAVF3lrLqp3FpUVsmC0t4lyVb8+FHBXeju+kYMCM1dyyPDIgsEhqoHVVZqNuIdLQ/YdOM= X-Received: by 10.236.222.9 with SMTP id s9mr2144237yhp.55.1421952309441; Thu, 22 Jan 2015 10:45:09 -0800 (PST) Received: from corpmail-nozzle1-2.hot.corp.google.com ([100.108.1.103]) by gmr-mx.google.com with ESMTPS id b3si667539qco.0.2015.01.22.10.45.08 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Jan 2015 10:45:09 -0800 (PST) Received: from ruffy2.mtv.corp.google.com ([172.17.128.107]) by corpmail-nozzle1-2.hot.corp.google.com with ESMTP id cwRyZf6E.1; Thu, 22 Jan 2015 10:45:09 -0800 From: Doug Evans MIME-Version: 1.0 Message-ID: <21697.17716.292890.813248@ruffy2.mtv.corp.google.com> Date: Thu, 22 Jan 2015 10:45:08 -0800 To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patchv3] Fix 100x slowdown regression on DWZ files In-Reply-To: <20150114202618.GA21056@host2.jankratochvil.net> References: <21548.37770.274873.760290@ruffy2.mtv.corp.google.com> <20141002155653.GA9001@host2.jankratochvil.net> <20141231192335.GA8188@host2.jankratochvil.net> <21677.57646.178793.836948@ruffy2.mtv.corp.google.com> <20150114202618.GA21056@host2.jankratochvil.net> X-IsSubscribed: yes 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. > > 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. > > 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 > > 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. */ }