From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2160 invoked by alias); 4 Jun 2008 22:27:34 -0000 Received: (qmail 2141 invoked by uid 22791); 4 Jun 2008 22:27:32 -0000 X-Spam-Check-By: sourceware.org Received: from cdptpa-omtalb.mail.rr.com (HELO cdptpa-omtalb.mail.rr.com) (75.180.132.120) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 04 Jun 2008 22:27:13 +0000 Received: from cdptpa-web12-z02 ([10.127.132.163]) by cdptpa-smta02.mail.rr.com with ESMTP id <20080604222709.HHRE14406.cdptpa-smta02.mail.rr.com@cdptpa-web12-z02>; Wed, 4 Jun 2008 22:27:09 +0000 Message-ID: <20001363.89401212618429743.JavaMail.root@cdptpa-web12-z02> Date: Wed, 04 Jun 2008 22:27:00 -0000 From: To: gdb-patches@sourceware.org Subject: PATCH: performance improvement in lookup_symtab() Cc: dbrueni@nc.rr.com MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sensitivity: Normal Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-06/txt/msg00027.txt.bz2 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 * 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);