diff mbox series

Avoid useless lookup_external_ref calls with LTO

Message ID alpine.LSU.2.20.1810311406440.1827@zhemvz.fhfr.qr
State New
Headers show
Series Avoid useless lookup_external_ref calls with LTO | expand

Commit Message

Richard Biener Oct. 31, 2018, 1:11 p.m. UTC 
Honza reports high CPU usage from lookup_external_ref at LTRANS time.
This is likely because build_abbrev_table calls it on all DIEs
rather than just type DIEs which we populate the ref table with.
So we end up calling it for all the DIEs refering to early debug,
possibly many distinct ones with the same die.symbol (but usually
different die_offset).  That will end up with loads of hashtable
collisions.

The following patch restricts the build_abbrev_table lookups to
type DIEs.

We might consider optimizing the refs to decrease the number of
relocations the link editor has to process but I guess it's not
worth it and the rest of the optimize_external_refs isn't prepared
for the LTO case anyways.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Honza has yet to confirm the patch helps, once he does I plan to
install it.

Richard.

2018-10-31  Richard Biener  <rguenther@suse.de>

	* dwarf2out.c (build_abbrev_table): Guard lookup_external_ref call
	with is_type_die.

Comments

Jan Hubicka Oct. 31, 2018, 3:49 p.m. UTC | #1 
> 
> Honza reports high CPU usage from lookup_external_ref at LTRANS time.
> This is likely because build_abbrev_table calls it on all DIEs
> rather than just type DIEs which we populate the ref table with.
> So we end up calling it for all the DIEs refering to early debug,
> possibly many distinct ones with the same die.symbol (but usually
> different die_offset).  That will end up with loads of hashtable
> collisions.
> 
> The following patch restricts the build_abbrev_table lookups to
> type DIEs.
> 
> We might consider optimizing the refs to decrease the number of
> relocations the link editor has to process but I guess it's not
> worth it and the rest of the optimize_external_refs isn't prepared
> for the LTO case anyways.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> Honza has yet to confirm the patch helps, once he does I plan to
> install it.

Before patch I got about 5% of lookup_external_ref while building
cc1 and after patch the function is off the profile :)

Honza

> 
> Richard.
> 
> 2018-10-31  Richard Biener  <rguenther@suse.de>
> 
> 	* dwarf2out.c (build_abbrev_table): Guard lookup_external_ref call
> 	with is_type_die.
> 
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 30bbfee9052..8b478aa265f 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -9023,8 +9023,9 @@ build_abbrev_table (dw_die_ref die, external_ref_hash_type *extern_map)
>  	struct external_ref *ref_p;
>  	gcc_assert (AT_ref (a)->comdat_type_p || AT_ref (a)->die_id.die_symbol);
>  
> -	ref_p = lookup_external_ref (extern_map, c);
> -	if (ref_p->stub && ref_p->stub != die)
> +	if (is_type_die (c)
> +	    && (ref_p = lookup_external_ref (extern_map, c))
> +	    && ref_p->stub && ref_p->stub != die)
>  	  change_AT_die_ref (a, ref_p->stub);
>  	else
>  	  /* We aren't changing this reference, so mark it external.  */
Jan Hubicka Dec. 15, 2018, 10:35 a.m. UTC | #2 
> 
> 2018-10-31  Richard Biener  <rguenther@suse.de>
> 
> 	* dwarf2out.c (build_abbrev_table): Guard lookup_external_ref call
> 	with is_type_die.

As discussed on IRC, i have backported this to GCC 8 because it causes
noticeable slowness when building Firefox with debug info.

Honza
diff mbox series

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 30bbfee9052..8b478aa265f 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -9023,8 +9023,9 @@  build_abbrev_table (dw_die_ref die, external_ref_hash_type *extern_map)
 	struct external_ref *ref_p;
 	gcc_assert (AT_ref (a)->comdat_type_p || AT_ref (a)->die_id.die_symbol);
 
-	ref_p = lookup_external_ref (extern_map, c);
-	if (ref_p->stub && ref_p->stub != die)
+	if (is_type_die (c)
+	    && (ref_p = lookup_external_ref (extern_map, c))
+	    && ref_p->stub && ref_p->stub != die)
 	  change_AT_die_ref (a, ref_p->stub);
 	else
 	  /* We aren't changing this reference, so mark it external.  */