* PATCH: performance improvement in lookup_symtab()
@ 2008-06-04 22:27 dbrueni
2008-06-04 22:48 ` Daniel Jacobowitz
0 siblings, 1 reply; 2+ messages in thread
From: dbrueni @ 2008-06-04 22:27 UTC (permalink / raw)
To: gdb-patches; +Cc: dbrueni
A customer of ours observed a noticable first-time
performance penalty setting breakpoints.
Since we are using absolute file paths to specify the
breakpoint, GDB was following a code path in lookup_symtab()
which was expensive because it was computing the fullpaths
and realpaths for every item in the symtabs and partial
symtabs as it scanned linearly for the matching file.
The customer had obvserved startup delays of up to 30
seconds on account of this issue. They report that there is no
delay when NOT sending absolute paths for breakpoint
file specs. However, sending unqualified file names is not a
general solution, it is really not a solution at all.
The attached patch optimizes the absolute path case by first
qualifying the symbol table entry's base file name against the
name we are searching for. If the base file name does not match,
then there is no point in calculating fullpaths or realpaths
for the symtab in question.
The only caveat to the changes is that it will potentially
fail to find a file that is symlinked to a symlink to a file
with a different name. That's not a typo -- a single symlink
will work because I test for the base file name both before
and after xfullpath() -- the file name has to be symlinked
twice to cause the search to fail. This is the case where:
FILENAME_CMP( lbasename( name ), lbasename( s->filename ) ) != 0
and
FILENAME_CMP( lbasename( xfullpath(name) ), lbasename( s->filename ) ) != 0
and
FILENAME_CMP( xfullpath(name), symtab_to_fullname(s) ) == 0
'name' basically has to be the middle symlink for this to fail.
In my books, this is an acceptable issue, considering that the
mainstream code path when the path was not absolute could not
deal with symlinks at all, and also considering that I'm making
the assumption that symtab_to_fullname(s->filename) would resolve
a symlink if there was one. Also, keep in mind, these are file
name symlinks, not directory symlinks, so this is not even
a remotely common use case, much less a chain of two such links.
I will have no objection if the extra comparison is
removed entirely and we just don't worry about symlinks at all.
Similar performance improvements to lookup_symtab() have been
suggested before, but never integrated. Knowing how expensive
symtab_to_fullname() can be, especially the cost of doing it for
EVERY single symbol table, I think this fix is pretty much a no-brainer.
GDB needs this, especially for the MI controlled case, where apps
are forced to send absolute file paths as that is the logical way
to handle setting breakpoints in same-named files in different
directories (like the example below):
/src/electronics/controller.cpp
/src/patterns/mvc/controller.cpp
/src/wife/controller.cpp
ChangeLog:
2008-06-03 Dennis Brueni <dbrueni@nc.rr.com>
* symtab.c (lookup_symtab and lookup_partial_symtab):
Optimized case where file name is an absolute path.
Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.189
diff -c -p -r1.189 symtab.c
*** symtab.c 27 May 2008 19:29:51 -0000 1.189
--- symtab.c 3 Jun 2008 21:43:33 -0000
*************** struct symtab *
*** 160,169 ****
--- 160,174 ----
lookup_symtab (const char *name)
{
struct symtab *s;
+ struct symtab *s_base = NULL;
struct partial_symtab *ps;
struct objfile *objfile;
char *real_path = NULL;
char *full_path = NULL;
+ const char *real_path_base = NULL;
+ const char *full_path_base = NULL;
+ const char *s_file_base = NULL;
+ const char *name_file_base = lbasename(name);
/* Here we are interested in canonicalizing an absolute path, not
absolutizing a relative path. */
*************** lookup_symtab (const char *name)
*** 173,178 ****
--- 178,185 ----
make_cleanup (xfree, full_path);
real_path = gdb_realpath (name);
make_cleanup (xfree, real_path);
+ real_path_base = lbasename(real_path);
+ full_path_base = lbasename(full_path);
}
got_symtab:
*************** got_symtab:
*** 185,225 ****
{
return s;
}
-
- /* If the user gave us an absolute path, try to find the file in
- this symtab and use its absolute path. */
! if (full_path != NULL)
! {
! const char *fp = symtab_to_fullname (s);
! if (fp != NULL && FILENAME_CMP (full_path, fp) == 0)
! {
! return s;
! }
}
! if (real_path != NULL)
{
! char *fullname = symtab_to_fullname (s);
! if (fullname != NULL)
! {
! char *rp = gdb_realpath (fullname);
! make_cleanup (xfree, rp);
! if (FILENAME_CMP (real_path, rp) == 0)
! {
! return s;
! }
! }
}
}
/* Now, search for a matching tail (only if name doesn't have any dirs) */
! if (lbasename (name) == name)
! ALL_SYMTABS (objfile, s)
{
! if (FILENAME_CMP (lbasename (s->filename), name) == 0)
! return s;
}
/* Same search rules as above apply here, but now we look thru the
--- 192,251 ----
{
return s;
}
! /* If the user gave us an absolute path, try to find the file in
! this symtab and use its absolute path.
! Note that if the base file name doesn't match, then there's
! no point in checking absolute or real paths
! */
! s_file_base = lbasename(s->filename);
!
! if (full_path != NULL)
! {
! /* compare both, in case if the file is symlink'd */
! if (FILENAME_CMP(full_path_base, s_file_base) == 0 ||
! FILENAME_CMP(name_file_base, s_file_base) == 0)
! {
! const char *fp = symtab_to_fullname (s);
! if (fp != NULL && FILENAME_CMP (full_path, fp) == 0)
! {
! return s;
! }
! }
! }
!
! if (real_path != NULL)
! {
! /* compare both, in case if the file is symlink'd */
! if (FILENAME_CMP(real_path_base, s_file_base) == 0 ||
! FILENAME_CMP(name_file_base, s_file_base) == 0)
! {
! char *fullname = symtab_to_fullname (s);
! if (fullname != NULL)
! {
! char *rp = gdb_realpath (fullname);
! make_cleanup (xfree, rp);
! if (FILENAME_CMP (real_path, rp) == 0)
! {
! return s;
! }
! }
! }
}
! /* Check if we might have an unqualified match */
! if (name_file_base==name && s_base==NULL)
{
! if (FILENAME_CMP (s_file_base, name) == 0)
! s_base = s;
}
}
/* Now, search for a matching tail (only if name doesn't have any dirs) */
! if (name_file_base==name && s_base!=NULL)
{
! return s_base;
}
/* Same search rules as above apply here, but now we look thru the
*************** struct partial_symtab *
*** 257,265 ****
--- 283,296 ----
lookup_partial_symtab (const char *name)
{
struct partial_symtab *pst;
+ struct partial_symtab *pst_base = NULL;
struct objfile *objfile;
char *full_path = NULL;
char *real_path = NULL;
+ const char *full_path_base = NULL;
+ const char *real_path_base = NULL;
+ const char *pst_file_base = NULL;
+ const char *name_file_base = lbasename(name);
/* Here we are interested in canonicalizing an absolute path, not
absolutizing a relative path. */
*************** lookup_partial_symtab (const char *name)
*** 269,274 ****
--- 300,307 ----
make_cleanup (xfree, full_path);
real_path = gdb_realpath (name);
make_cleanup (xfree, real_path);
+ full_path_base = lbasename(full_path);
+ real_path_base = lbasename(real_path);
}
ALL_PSYMTABS (objfile, pst)
*************** lookup_partial_symtab (const char *name)
*** 279,318 ****
}
/* If the user gave us an absolute path, try to find the file in
! this symtab and use its absolute path. */
! if (full_path != NULL)
! {
! psymtab_to_fullname (pst);
! if (pst->fullname != NULL
! && FILENAME_CMP (full_path, pst->fullname) == 0)
{
! return pst;
}
}
! if (real_path != NULL)
! {
! char *rp = NULL;
! psymtab_to_fullname (pst);
! if (pst->fullname != NULL)
! {
! rp = gdb_realpath (pst->fullname);
! make_cleanup (xfree, rp);
! }
! if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
{
! return pst;
}
}
}
/* Now, search for a matching tail (only if name doesn't have any dirs) */
!
! if (lbasename (name) == name)
! ALL_PSYMTABS (objfile, pst)
{
! if (FILENAME_CMP (lbasename (pst->filename), name) == 0)
! return (pst);
}
return (NULL);
--- 312,370 ----
}
/* If the user gave us an absolute path, try to find the file in
! this symtab and use its absolute path.
! Note that if the base file name doesn't match, then there's
! no point in checking absolute or real paths
! */
! pst_file_base = lbasename(pst->filename);
!
! if (full_path != NULL)
! {
! /* compare both, in case if the file is symlink'd */
! if (FILENAME_CMP(full_path_base, pst_file_base) == 0 ||
! FILENAME_CMP(name_file_base, pst_file_base) == 0)
{
! psymtab_to_fullname (pst);
! if (pst->fullname != NULL
! && FILENAME_CMP (full_path, pst->fullname) == 0)
! {
! return pst;
! }
}
}
! if (real_path != NULL)
! {
! /* compare both, in case if the file is symlink'd */
! if (FILENAME_CMP(real_path_base, pst_file_base) == 0 ||
! FILENAME_CMP(name_file_base, pst_file_base) == 0)
{
! char *rp = NULL;
! psymtab_to_fullname (pst);
! if (pst->fullname != NULL)
! {
! rp = gdb_realpath (pst->fullname);
! make_cleanup (xfree, rp);
! }
! if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
! {
! return pst;
! }
}
}
+
+ /* Now, check for a matching tail (only if name doesn't have any dirs) */
+ if (name_file_base==name && pst_base==NULL)
+ {
+ if (FILENAME_CMP (pst_file_base, name) == 0)
+ return (pst);
+ }
}
/* Now, search for a matching tail (only if name doesn't have any dirs) */
! if (name_file_base==name && pst_base!=NULL)
{
! return pst_base;
}
return (NULL);
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: PATCH: performance improvement in lookup_symtab()
2008-06-04 22:27 PATCH: performance improvement in lookup_symtab() dbrueni
@ 2008-06-04 22:48 ` Daniel Jacobowitz
0 siblings, 0 replies; 2+ messages in thread
From: Daniel Jacobowitz @ 2008-06-04 22:48 UTC (permalink / raw)
To: dbrueni; +Cc: gdb-patches
Hi, and thanks for contributing. I'm interested in this issue too.
On Wed, Jun 04, 2008 at 06:27:09PM -0400, dbrueni@nc.rr.com wrote:
> /src/electronics/controller.cpp
> /src/patterns/mvc/controller.cpp
> /src/wife/controller.cpp
I love this example.
Unfortunately, this is a substantial patch. That means we can't
accept it unless it is covered by a copyright assignment. I don't see
your name in the FSF's list; do you or your employer have an
assignment in place? If not, I can send you the form.
I'm not too concerned about the symlink issue, though I do want to
take a closer look at it once we have assignment sorted out.
--
Daniel Jacobowitz
CodeSourcery
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2008-06-04 22:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-06-04 22:27 PATCH: performance improvement in lookup_symtab() dbrueni
2008-06-04 22:48 ` Daniel Jacobowitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox