[patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jan Kratochvil]

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.

--- 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);

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tristan Gingold]

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.

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jan Kratochvil]

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

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jan Kratochvil]

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

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tristan Gingold]

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.

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tom Tromey]

>>>>> "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

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tristan Gingold]

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.

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tom Tromey]

>>>>> "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

Re: [patch] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Tristan Gingold]

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.

[patch#2] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jan Kratochvil]

On Tue, 18 Oct 2011 09:24:08 +0200, Tristan Gingold wrote:
> 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.

I find an extra option just a pollution of doc and everything by an option
which will never get used.  Wouldn't it be enough to disable it by
-gstrict-dwarf?  While currently the -gstrict-dwarf meaning is different
I believe the purpose is correct - to be more backward compatible.


Thanks,
Jan


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

	Stop producing DW_AT_sibling without -gstrict-dwarf.
	* dwarf2out.c (dwarf2out_finish): Remove calls of
	add_sibling_attributes if !DWARF_STRICT.  Extend the comment with
	reason.

--- gcc/dwarf2out.c	(revision 180121)
+++ gcc/dwarf2out.c	(working copy)
@@ -22496,13 +22496,17 @@ 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);
+  if (dwarf_strict)
+    {
+      /* Traverse the DIE's and add add sibling attributes to those DIE's that
+	 have children.  It is produced only for compatibility reasons as it is
+	 both a size and consumer performance hit.  */
+      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);

Re: [patch#2] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jakub Jelinek]

On Tue, Oct 18, 2011 at 10:28:09AM +0200, Jan Kratochvil wrote:
> 2011-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Stop producing DW_AT_sibling without -gstrict-dwarf.
> 	* dwarf2out.c (dwarf2out_finish): Remove calls of
> 	add_sibling_attributes if !DWARF_STRICT.  Extend the comment with
> 	reason.

This is ok for trunk.

	Jakub

Re: [patch#2] dwarf2out: Drop the size + performance overhead of DW_AT_sibling

[Jan Kratochvil]

On Tue, 18 Oct 2011 10:38:23 +0200, Jakub Jelinek wrote:
> On Tue, Oct 18, 2011 at 10:28:09AM +0200, Jan Kratochvil wrote:
> > 2011-10-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Stop producing DW_AT_sibling without -gstrict-dwarf.
> > 	* dwarf2out.c (dwarf2out_finish): Remove calls of
> > 	add_sibling_attributes if !DWARF_STRICT.  Extend the comment with
> > 	reason.
> 
> This is ok for trunk.

FYI this patch has not yet been checked in, it has negative performance effect
on the systemtap DWARF consumer.
	http://sourceware.org/ml/archer/2011-q4/msg00004.html

I will post a patch removing only very short DW_AT_sibling skips later.


Thanks,
Jan