diff mbox

dwarf2out: Drop the size + performance overhead of DW_AT_sibling

Message ID 20111012135008.GA24037@host1.jankratochvil.net
State New
Headers show

Commit Message

Jan Kratochvil Oct. 12, 2011, 1:50 p.m. UTC 
Hi,

dropping the optional DWARF attribute DW_AT_sibling has only advantages and no
disadvantages:

For files with .gdb_index GDB initial scan does not use DW_AT_sibling at all.
For files without .gdb_index GDB initial scan has 1.79% time _improvement_.
For .debug files it brings 3.49% size decrease (7.84% for rpm compressed files).

I guess DW_AT_sibling had real performance gains on CPUs with 1x (=no) clock
multipliers.  Nowadays mostly only the data size transferred over FSB matters.

I do not think there would be any DWARF consumers compatibility problems as
DW_AT_sibling has always been optional but I admit I have tested only GDB.


"clean" is FSF GCC+GDB, "ns" is FSF GCC with the patch applied.

gdbindex -readnow 100x warm:
clean:
56.975 57.161 57.738 58.243 57.52924999999999 seconds
ns:
57.799 58.008 58.202 58.473 58.120499999999993 seconds
+1.03% = performance decrease but it should be 0%, it is a measurement error
gdbindex -readnow 20x warm(gdb) cold(data):
clean:
57.989
ns:
58.538
+0.95% = performance decrease but it should be 0%, it is a measurement error
200x warm:
clean:
14.393 14.414 14.587 14.496 14.472499999999998 seconds
ns:
14.202 14.160 14.174 14.318 14.213499999999998 seconds
-1.79% = performance improvement of non-gdbindex scan (dwarf2_build_psymtabs_hard)

gdbindex .debug:
clean = 5589272 bytes
ns = 5394120 bytes
-3.49% = size improvement

gdbindex .debug.xz9:
clean = 1158696 bytes
ns = 1067900 bytes
-7.84% = size improvement

.debug_info + .debug_types:
clean = 0x1a11a0+0x08f389 bytes
ns = 0x184205+0x0833b0 bytes
-7.31% = size improvement

Intel i7-920 CPU and only libstdc++ from GCC 4.7.0 20111002 and `-O2 -gdwarf-4
-fdebug-types-section' were used for the benchmark.  GCC 4.7.0 20111002
--enable-languages=c++ was used for `make check' regression testing.


Thanks,
Jan


gcc/
2011-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Stop producing DW_AT_sibling.
	* dwarf2out.c (add_sibling_attributes): Remove the declaration.
	(add_sibling_attributes): Remove the function.
	(dwarf2out_finish): Remove calls of add_sibling_attributes.

Comments

Tristan Gingold Oct. 12, 2011, 2:07 p.m. UTC | #1 
On Oct 12, 2011, at 3:50 PM, Jan Kratochvil wrote:

> Hi,
> 
> dropping the optional DWARF attribute DW_AT_sibling has only advantages and no
> disadvantages:
> 
> For files with .gdb_index GDB initial scan does not use DW_AT_sibling at all.
> For files without .gdb_index GDB initial scan has 1.79% time _improvement_.
> For .debug files it brings 3.49% size decrease (7.84% for rpm compressed files).
> 
> I guess DW_AT_sibling had real performance gains on CPUs with 1x (=no) clock
> multipliers.  Nowadays mostly only the data size transferred over FSB matters.
> 
> I do not think there would be any DWARF consumers compatibility problems as
> DW_AT_sibling has always been optional but I admit I have tested only GDB.

I fear that this may degrade performance of other debuggers.  What about adding a command line option ?

Tristan.
Jan Kratochvil Oct. 12, 2011, 2:18 p.m. UTC | #2 
On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote:
> I fear that this may degrade performance of other debuggers.  What about
> adding a command line option ?

I can test idb, there aren't so many DWARF debuggers out there I think.

If the default is removed DW_AT_sibling a new options may make sense as some
compatibility safeguard.


Thanks,
Jan
Jan Kratochvil Oct. 13, 2011, 8:40 p.m. UTC | #3 
On Wed, 12 Oct 2011 16:18:07 +0200, Jan Kratochvil wrote:
> On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote:
> > I fear that this may degrade performance of other debuggers.  What about
> > adding a command line option ?
> 
> I can test idb,

I do not find the difference measurable.  Dropping DW_AT_sibling is 0.25%
performance _improvement_ but I guess it is just less than the measurement
error.

libstdc++ built with gcc -gdwarf-2 as with gcc -gdwarf-4 -fdebug-types-section
it crashes.  i7-920 x86_64 used for testing:
Intel(R) Debugger for applications running on Intel(R) 64, Version 12.1, Build [76.472.14]

with DW_AT_sibling
real    2m34.206s 2m31.822s 2m31.709s 2m32.316s
avg = 152.51325 seconds

patched GCC without DW_AT_sibling
real    2m32.528s 2m30.524s 2m33.767s 2m31.719s
avg = 152.1345 seconds

I do not see a point in keeping DW_AT_sibling there.


Regards,
Jan
Tristan Gingold Oct. 14, 2011, 8:29 a.m. UTC | #4 
On Oct 13, 2011, at 10:40 PM, Jan Kratochvil wrote:

> On Wed, 12 Oct 2011 16:18:07 +0200, Jan Kratochvil wrote:
>> On Wed, 12 Oct 2011 16:07:24 +0200, Tristan Gingold wrote:
>>> I fear that this may degrade performance of other debuggers.  What about
>>> adding a command line option ?
>> 
>> I can test idb,
> 
> I do not find the difference measurable.  Dropping DW_AT_sibling is 0.25%
> performance _improvement_ but I guess it is just less than the measurement
> error.
> 
> libstdc++ built with gcc -gdwarf-2 as with gcc -gdwarf-4 -fdebug-types-section
> it crashes.  i7-920 x86_64 used for testing:
> Intel(R) Debugger for applications running on Intel(R) 64, Version 12.1, Build [76.472.14]
> 
> with DW_AT_sibling
> real    2m34.206s 2m31.822s 2m31.709s 2m32.316s
> avg = 152.51325 seconds
> 
> patched GCC without DW_AT_sibling
> real    2m32.528s 2m30.524s 2m33.767s 2m31.719s
> avg = 152.1345 seconds
> 
> I do not see a point in keeping DW_AT_sibling there.

I am not against this patch, my only concern is that there are many many dwarf consumers and I have no idea how they will react to this change.

Tristan.
Tom Tromey Oct. 14, 2011, 2:02 p.m. UTC | #5 
>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:

Tristan> I am not against this patch, my only concern is that there are many
Tristan> many dwarf consumers and I have no idea how they will react to this
Tristan> change.

I tend to think that this is the wrong standard to apply.  In this case
we would be avoiding a beneficial change -- as measured in both
performance in a couple of cases, and in size -- for the sake of unknown
and possibly nonexistent consumers.  I think instead the burden of proof
should be on those consumers, both to give their evidence and reasoning
and to engage with GCC.

Another way to look at it is that there have been many changes to GCC's
DWARF output in the last few years.  Surely these have broken these
DWARF consumers more than this change possibly could.

Tom
Tristan Gingold Oct. 17, 2011, 7:20 a.m. UTC | #6 
On Oct 14, 2011, at 4:02 PM, Tom Tromey wrote:

>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
> 
> Tristan> I am not against this patch, my only concern is that there are many
> Tristan> many dwarf consumers and I have no idea how they will react to this
> Tristan> change.
> 
> I tend to think that this is the wrong standard to apply.  In this case
> we would be avoiding a beneficial change -- as measured in both
> performance in a couple of cases, and in size --

I am not against this patch.  I think it would be useful to add an option (-fdwarf-emit-sibling ?) to keep the old behavior.

> for the sake of unknown
> and possibly nonexistent consumers.  I think instead the burden of proof
> should be on those consumers, both to give their evidence and reasoning
> and to engage with GCC.

You know the story here:  they don't use the latest gcc version and start to complain years later.

> Another way to look at it is that there have been many changes to GCC's
> DWARF output in the last few years.  Surely these have broken these
> DWARF consumers more than this change possibly could.

Yes, but there is -gstrict-dwarf to stay compatible with old behavior.

Tristan.
Tom Tromey Oct. 17, 2011, 1:16 p.m. UTC | #7 
>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:

Tom> Another way to look at it is that there have been many changes to GCC's
Tom> DWARF output in the last few years.  Surely these have broken these
Tom> DWARF consumers more than this change possibly could.

Tristan> Yes, but there is -gstrict-dwarf to stay compatible with old behavior.

Yes, but this change is strictly compliant.  What makes it different
from any other change that was made to make GCC more compliant?
Hypothetical consumers could also break on those changes.

Tom
Tristan Gingold Oct. 18, 2011, 7:24 a.m. UTC | #8 
On Oct 17, 2011, at 3:16 PM, Tom Tromey wrote:

>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
> 
> Tom> Another way to look at it is that there have been many changes to GCC's
> Tom> DWARF output in the last few years.  Surely these have broken these
> Tom> DWARF consumers more than this change possibly could.
> 
> Tristan> Yes, but there is -gstrict-dwarf to stay compatible with old behavior.
> 
> Yes, but this change is strictly compliant.

Agreed.

>  What makes it different
> from any other change that was made to make GCC more compliant?
> Hypothetical consumers could also break on those changes.

The others changes were often corner cases, while this one is very visible.

What is wrong with my suggestion of adding a command line option to keep the siblings ?  This option could be removed in a few years if nobody complained about sibling removal.

Tristan.
diff mbox

Patch

--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -3316,7 +3316,6 @@  static int htab_cu_eq (const void *, const void *);
 static void htab_cu_del (void *);
 static int check_duplicate_cu (dw_die_ref, htab_t, unsigned *);
 static void record_comdat_symbol_number (dw_die_ref, htab_t, unsigned);
-static void add_sibling_attributes (dw_die_ref);
 static void build_abbrev_table (dw_die_ref);
 static void output_location_lists (dw_die_ref);
 static int constant_size (unsigned HOST_WIDE_INT);
@@ -7482,24 +7481,6 @@  copy_decls_for_unworthy_types (dw_die_ref unit)
   unmark_dies (unit);
 }
 
-/* Traverse the DIE and add a sibling attribute if it may have the
-   effect of speeding up access to siblings.  To save some space,
-   avoid generating sibling attributes for DIE's without children.  */
-
-static void
-add_sibling_attributes (dw_die_ref die)
-{
-  dw_die_ref c;
-
-  if (! die->die_child)
-    return;
-
-  if (die->die_parent && die != die->die_parent->die_child)
-    add_AT_die_ref (die, DW_AT_sibling, die->die_sib);
-
-  FOR_EACH_CHILD (die, c, add_sibling_attributes (c));
-}
-
 /* Output all location lists for the DIE and its children.  */
 
 static void
@@ -22496,14 +22477,6 @@  dwarf2out_finish (const char *filename)
       prune_unused_types ();
     }
 
-  /* Traverse the DIE's and add add sibling attributes to those DIE's
-     that have children.  */
-  add_sibling_attributes (comp_unit_die ());
-  for (node = limbo_die_list; node; node = node->next)
-    add_sibling_attributes (node->die);
-  for (ctnode = comdat_type_list; ctnode != NULL; ctnode = ctnode->next)
-    add_sibling_attributes (ctnode->root_die);
-
   /* Output a terminator label for the .text section.  */
   switch_to_section (text_section);
   targetm.asm_out.internal_label (asm_out_file, TEXT_END_LABEL, 0);