diff mbox

Make file-based lookup more interruptable

Message ID 089e0160b1be9352da05162640cb@google.com
State New
Headers show

Commit Message

Doug Evans May 15, 2015, 10:20 p.m. UTC
Hi.

This patch makes, for example, "b 'foo.c':bar" more interruptable.

I put the QUIT calls at two logical levels for robustness and clarity:
iterate_over_symtabs and map_symtabs_matching_filename.
I plan to do the same for other QUIT calls I've already added
and plan to add.

2015-05-15  Doug Evans  <dje@google.com>

	* completer.c (location_completer): Protect file_to_match with cleanup.
	* dwarf2read.c (dw2_map_symtabs_matching_filename): Add QUIT calls.
	* psymtab.c (psym_map_symtabs_matching_filename): Add QUIT call.
	* symtab.c (iterate_over_symtabs): Add QUIT calls.

Comments

Yao Qi May 19, 2015, 11:55 a.m. UTC | #1
Doug Evans <dje@google.com> writes:

> This patch makes, for example, "b 'foo.c':bar" more interruptable.

Does 'interruptable' mean user can type ctrl-c to cancel the command
"b 'foo.c':bar" which may take 10 minutes?  If so, this change is
visible to users, and do we need to add a NEWS entry?
Doug Evans May 22, 2015, 5:46 p.m. UTC | #2
On Tue, May 19, 2015 at 4:55 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Doug Evans <dje@google.com> writes:
>
>> This patch makes, for example, "b 'foo.c':bar" more interruptable.
>
> Does 'interruptable' mean user can type ctrl-c to cancel the command
> "b 'foo.c':bar" which may take 10 minutes?  If so, this change is
> visible to users, and do we need to add a NEWS entry?

I'd classify this as being no different than a performance improvement.
There already is a level of interruptability, the only change is in degree
of responsiveness.

We *can* start adding such things to NEWS, but we don't do so today.
Before we start imposing this on everyone, we'll need to establish
what does and does not qualify - I'm guessing we don't want to add
a NEWS entry for every little performance improvement.
Doug Evans June 2, 2015, 12:03 a.m. UTC | #3
On Fri, May 22, 2015 at 10:46 AM, Doug Evans <dje@google.com> wrote:
> On Tue, May 19, 2015 at 4:55 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> Doug Evans <dje@google.com> writes:
>>
>>> This patch makes, for example, "b 'foo.c':bar" more interruptable.
>>
>> Does 'interruptable' mean user can type ctrl-c to cancel the command
>> "b 'foo.c':bar" which may take 10 minutes?  If so, this change is
>> visible to users, and do we need to add a NEWS entry?
>
> I'd classify this as being no different than a performance improvement.
> There already is a level of interruptability, the only change is in degree
> of responsiveness.
>
> We *can* start adding such things to NEWS, but we don't do so today.
> Before we start imposing this on everyone, we'll need to establish
> what does and does not qualify - I'm guessing we don't want to add
> a NEWS entry for every little performance improvement.

Hi.

"ping" :-)

Whaddayathink? Should we start adding perf improvements to NEWS
(and try to classify what is newsworthy)?
Or let it go, at least for now?
Yao Qi June 2, 2015, 7:59 a.m. UTC | #4
[Sorry for being late]

On 22/05/15 18:46, Doug Evans wrote:
> I'd classify this as being no different than a performance improvement.

I think they are different.  IMO, performance improvement is the time
or memory reduction to some degree.  This kind of change can be user
visible (10 min -> 10s) and can be user invisible (2s -> 1s).

> There already is a level of interruptability, the only change is in degree
> of responsiveness.
>
> We*can*  start adding such things to NEWS, but we don't do so today.
> Before we start imposing this on everyone, we'll need to establish
> what does and does not qualify - I'm guessing we don't want to add
> a NEWS entry for every little performance improvement.

However, this change isn't performance improvement, because it doesn't
shorten the time of operation like "b 'foo.c':bar".  This change only
makes GDB more interruptable during some operations, IMO this is a good
news to users and we should let them know.

Regarding to NEWS entry for performance improvement, I don't have an
opinion on it.
Doug Evans June 2, 2015, 12:34 p.m. UTC | #5
On Tue, Jun 2, 2015 at 12:59 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> This kind of change [a perf improvement can be user
> visible (10 min -> 10s) and can be user invisible (2s -> 1s).

An improvement of 10min -> 10s (or greater, I have made several)
would be "good news to users", no?

> However, this change isn't performance improvement, because it doesn't
> shorten the time of operation like "b 'foo.c':bar".

Oh?
"b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
How is this any different?

> This change only
> makes GDB more interruptable during some operations, IMO this is a good
> news to users and we should let them know.
Yao Qi June 2, 2015, 1:08 p.m. UTC | #6
On 02/06/15 13:34, Doug Evans wrote:
> An improvement of 10min -> 10s (or greater, I have made several)
> would be "good news to users", no?

They are good news to users, of course.  However, whether letting users
know them is another thing we need to decide.

>
>> >However, this change isn't performance improvement, because it doesn't
>> >shorten the time of operation like "b 'foo.c':bar".
> Oh?
> "b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
> How is this any different?
>

Operation "b 'foo.c':bar" becomes more interruptable rather than more
efficient.
Doug Evans June 2, 2015, 1:20 p.m. UTC | #7
On Tue, Jun 2, 2015 at 6:08 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>>> >However, this change isn't performance improvement, because it doesn't
>>> >shorten the time of operation like "b 'foo.c':bar".
>>
>> Oh?
>> "b 'foo.c':bar" + ^c changes from 10min to 10sec (or some such).
>> How is this any different?
>>
>
> Operation "b 'foo.c':bar" becomes more interruptable rather than more
> efficient.

Eh?
I'm sorry but the difference is just lost on me.

Ok, how about we add a rule saying that the addition of any calls to
QUIT shall require a NEWS entry.
[Since that is what you are requesting.]
Yao Qi June 2, 2015, 1:46 p.m. UTC | #8
On 02/06/15 14:20, Doug Evans wrote:
> Eh?
> I'm sorry but the difference is just lost on me.
>
> Ok, how about we add a rule saying that the addition of any calls to
> QUIT shall require a NEWS entry.
> [Since that is what you are requesting.]

Hi Doug,
I don't insist on adding a NEWS entry for this change.  If it is not
necessary to do so for you, I am OK too.

The intention of my suggestion of adding NEWS entry for this change is
to show the good improvements to users as much as we can.
diff mbox

Patch

diff --git a/gdb/completer.c b/gdb/completer.c
index c8c0e4c..c796951 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -195,7 +195,6 @@  location_completer (struct cmd_list_element *ignore,
    int quoted = *text == '\'' || *text == '"';
    int quote_char = '\0';
    const char *colon = NULL;
-  char *file_to_match = NULL;
    const char *symbol_start = text;
    const char *orig_text = text;
    size_t text_len;
@@ -241,12 +240,17 @@  location_completer (struct cmd_list_element *ignore,
      text++;
    text_len = strlen (text);

-  /* Where is the file name?  */
-  if (colon)
+  /* Where is the file name?
+     If the text includes a colon, they want completion only on a
+     symbol name after the colon.  Otherwise, we need to complete on
+     symbols as well as on files.  */
+  if (colon != NULL)
      {
-      char *s;
+      char *s, *file_to_match;
+      struct cleanup *cleanup;

-      file_to_match = (char *) xmalloc (colon - text + 1);
+      file_to_match = xmalloc (colon - text + 1);
+      cleanup = make_cleanup (xfree, file_to_match);
        strncpy (file_to_match, text, colon - text + 1);
        /* Remove trailing colons and quotes from the file name.  */
        for (s = file_to_match + (colon - text);
@@ -254,15 +258,9 @@  location_completer (struct cmd_list_element *ignore,
  	   s--)
  	if (*s == ':' || *s == quote_char)
  	  *s = '\0';
-    }
-  /* If the text includes a colon, they want completion only on a
-     symbol name after the colon.  Otherwise, we need to complete on
-     symbols as well as on files.  */
-  if (colon)
-    {
        list = make_file_symbol_completion_list (symbol_start, word,
  					       file_to_match);
-      xfree (file_to_match);
+      do_cleanups (cleanup);
      }
    else
      {
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4d892d8..ca8573a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3448,6 +3448,8 @@  dw2_map_symtabs_matching_filename (struct objfile  
*objfile, const char *name,
        struct dwarf2_per_cu_data *per_cu = dw2_get_cu (i);
        struct quick_file_names *file_data;

+      QUIT;
+
        /* We only need to look at symtabs not already expanded.  */
        if (per_cu->v.quick->compunit_symtab)
  	continue;
@@ -3461,6 +3463,8 @@  dw2_map_symtabs_matching_filename (struct objfile  
*objfile, const char *name,
  	  const char *this_name = file_data->file_names[j];
  	  const char *this_real_name;

+	  QUIT;
+
  	  if (compare_filenames_for_search (this_name, name))
  	    {
  	      if (dw2_map_expand_apply (objfile, per_cu, name, real_path,
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 383e4c4..27ba2ba 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -169,6 +169,8 @@  psym_map_symtabs_matching_filename (struct objfile  
*objfile,

    ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
    {
+    QUIT;
+
      /* We can skip shared psymtabs here, because any file name will be
         attached to the unshared psymtab.  */
      if (pst->user != NULL)
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 72df872..f06622a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -443,6 +443,8 @@  iterate_over_symtabs (const char *name,

    ALL_OBJFILES (objfile)
    {
+    QUIT;
+
      if (iterate_over_some_symtabs (name, real_path, callback, data,
  				   objfile->compunit_symtabs, NULL))
        {
@@ -456,6 +458,8 @@  iterate_over_symtabs (const char *name,

    ALL_OBJFILES (objfile)
    {
+    QUIT;
+
      if (objfile->sf
  	&& objfile->sf->qf->map_symtabs_matching_filename (objfile,
  							   name,