[Bug symtab/27937] New: Performance regression of 8% due to assert in dwarf/read/attribute.h

[Bug symtab/27937] New: Performance regression of 8% due to assert in dwarf/read/attribute.h

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

            Bug ID: 27937
           Summary: Performance regression of 8% due to assert in
                    dwarf/read/attribute.h
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: symtab
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

PR23710 is about gdb being slow for executables produced using gcc and LTO.

A cc1 exec is mentioned, and this gdb invocation using it as being slow:
...
$ gdb -q -batch cc1 -ex "b do_rpo_vn"
...

I've measured performance using recent releases build with -O2 on openSUSE Leap
15.2, as well as HEAD@3633d4fb446 (2021-05-28):
...
+----------+-------+---------+
| version  | real  | maxrss  |
+----------+-------+---------+
|  8.1.1   | 9.42s | 1443980 |
|  8.2.1   |     - |       - |
|  8.3.1   | 9.31s | 1460376 |
|  9.2     | 8.50s | 1495064 |
| 10.2     | 5.86s |  872584 |
| HEAD     | 6.36s |  871960 |
+----------+-------+---------+
...
[ No results in 8.2.1 due to PR23712. ]

There's clear progress going towards 10.2, but after 10.2 we regress, with
8.5%.

I've bisected this to:
...
commit 1bc397c561f55c1cf67a5e02f5c9305a54155a2e
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Sep 29 18:49:08 2020 -0600

    Remove DW_SND

    This removes DW_SND in favor of accessors on struct attribute.
    These accessors check that the form is appropriate.
...

Commenting out this assert:
...
$ git diff
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 91782769689..83caef8e92d 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -69,7 +69,7 @@ struct attribute
      form.  */
   LONGEST as_signed () const
   {
-    gdb_assert (form == DW_FORM_sdata || form == DW_FORM_implicit_const);
+    //gdb_assert (form == DW_FORM_sdata || form == DW_FORM_implicit_const);
     return u.snd;
   }

...
gets me to 5.90s.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[ssbssa at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

Hannes Domani <ssbssa at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ssbssa at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simark at simark dot ca

--- Comment #1 from Simon Marchi <simark at simark dot ca> ---
I sometimes wonder if we should have a gdb_assert version that is only enabled
in development builds, that we could use in very hot paths like this one,
without affecting performance of production builds.  On one hand, it's not good
because problems could go unnoticed in production builds.  But if the
alternative is removing the assert because it is too costly, then having it
only in development builds is better than nothing.

Also, I was wondering if this would have any effect on your measurements:

diff --git a/gdbsupport/gdb_assert.h b/gdbsupport/gdb_assert.h
index 00553a786135..74dcd653cc4e 100644
--- a/gdbsupport/gdb_assert.h
+++ b/gdbsupport/gdb_assert.h
@@ -32,7 +32,7 @@
    replacing.  */

 #define gdb_assert(expr)                                                     
\
-  ((void) ((expr) ? 0 :                                                      
\
+  ((void) (__builtin_expect ((expr), 0) ? 0 :                                 
                     \
           (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0)))

 /* This prints an "Assertion failed" message, asking the user if they

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

--- Comment #2 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Simon Marchi from comment #1)
> I sometimes wonder if we should have a gdb_assert version that is only
> enabled in development builds, that we could use in very hot paths like this
> one, without affecting performance of production builds.

Seems reasonable to me, and especially for this assert, which just introduces
a new check that didn't even exist in earlier versions.  That is, removing
the assert or having it not run in production is identical to the status quo
ante.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Tom Tromey from comment #2)
> (In reply to Simon Marchi from comment #1)
> > I sometimes wonder if we should have a gdb_assert version that is only
> > enabled in development builds, that we could use in very hot paths like this
> > one, without affecting performance of production builds.
> 
> Seems reasonable to me, and especially for this assert, which just introduces
> a new check that didn't even exist in earlier versions.  That is, removing
> the assert or having it not run in production is identical to the status quo
> ante.

After writing this, I remembered that Pedro hadn't like the 
"asan-by-default" approach, because it meant that you had to
do a special build to do performance testing.
So maybe we need to have opt-in development asserts.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

--- Comment #4 from Tom Tromey <tromey at sourceware dot org> ---
I'm trying a debug assert that checks _GLIBCXX_DEBUG

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/27937] Performance regression of 8% due to assert in dwarf/read/attribute.h

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=27937

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
A simple test like:

/bin/time -f%e ~/gdb/install/bin/gdb -batch -nx -ex 'break internal_error'
/tmp/gdb

... does not show any improvement with this assert commented out.
So I wonder if it's still worthwhile.

Commenting out all the asserts in attribute.h does increase
that test's speed a little bit -- from ~2.06 to ~2.03 with a hot cache.

https://github.com/tromey/gdb/commit/b3ed9b13eb970972b72842644a894d95b02b71b0

-- 
You are receiving this mail because:
You are on the CC list for the bug.