[PATCH] Instantiate a single source highlighter

[PATCH] Instantiate a single source highlighter

[Tom Tromey]

It occurred to me that there's no reason to make a new source
highlighter each time gdb needs to highlight some source code.
Instead, a single one can be created and then simply reused each time.

This patch implements this idea.  Tested on x86-64 Fedora 29.

gdb/ChangeLog
2019-06-18  Tom Tromey  <tromey@adacore.com>

	* source-cache.c (highlighter): New global.
	(source_cache::get_source_lines): Create a highlighter on demand.
---
 gdb/ChangeLog      |  5 +++++
 gdb/source-cache.c | 19 ++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 2d5b549d971..0fa456116b4 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -33,6 +33,7 @@
 #include <sstream>
 #include <srchilite/sourcehighlight.h>
 #include <srchilite/langmap.h>
+#include <srchilite/parserexception.h>
 #endif
 
 /* The number of source files we'll cache.  */
@@ -43,6 +44,14 @@
 
 source_cache g_source_cache;
 
+/* The global source highlight object, or null if one was never
+   constructed.  This is stored here rather than in the class so that
+   we don't need to include anything or do conditional compilation in
+   source-cache.h.  */
+#ifdef HAVE_SOURCE_HIGHLIGHT
+static srchilite::SourceHighlight *highlighter;
+#endif
+
 /* See source-cache.h.  */
 
 bool
@@ -209,11 +218,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
 		     use-after-free.  */
 		  fullname = symtab_to_fullname (s);
 		}
-	      srchilite::SourceHighlight highlighter ("esc.outlang");
-	      highlighter.setStyleFile("esc.style");
+
+	      if (highlighter == nullptr)
+		{
+		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
+		  highlighter->setStyleFile("esc.style");
+		}
 
 	      std::ostringstream output;
-	      highlighter.highlight (input, output, lang_name, fullname);
+	      highlighter->highlight (input, output, lang_name, fullname);
 
 	      source_text result = { fullname, output.str () };
 	      m_source_map.push_back (std::move (result));
-- 
2.20.1

Re: [PATCH] Instantiate a single source highlighter

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> +#include <srchilite/parserexception.h>

Oops, I didn't mean to include this hunk.

This came from an attempt to let gdb try to use the rust language from
source highlight (new in git -- not in a release), and then fall back if
it isn't found.  However, that's causing crashes in the library and so
isn't quite ready...

Meanwhile I've removed this include from my patch.

Tom

Re: [PATCH] Instantiate a single source highlighter

[Pedro Alves]

On 6/18/19 8:40 PM, Tom Tromey wrote:
> It occurred to me that there's no reason to make a new source
> highlighter each time gdb needs to highlight some source code.
> Instead, a single one can be created and then simply reused each time.
> 
> This patch implements this idea.  Tested on x86-64 Fedora 29.
> 

Assuming we won't need this from different threads anytime soon,
seems fine.  Comments below.

> gdb/ChangeLog
> 2019-06-18  Tom Tromey  <tromey@adacore.com>
> 
> 	* source-cache.c (highlighter): New global.
> 	(source_cache::get_source_lines): Create a highlighter on demand.
> ---
>  gdb/ChangeLog      |  5 +++++
>  gdb/source-cache.c | 19 ++++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 2d5b549d971..0fa456116b4 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -33,6 +33,7 @@
>  #include <sstream>
>  #include <srchilite/sourcehighlight.h>
>  #include <srchilite/langmap.h>
> +#include <srchilite/parserexception.h>
>  #endif
>  
>  /* The number of source files we'll cache.  */
> @@ -43,6 +44,14 @@
>  
>  source_cache g_source_cache;
>  
> +/* The global source highlight object, or null if one was never
> +   constructed.  This is stored here rather than in the class so that
> +   we don't need to include anything or do conditional compilation in
> +   source-cache.h.  */
> +#ifdef HAVE_SOURCE_HIGHLIGHT
> +static srchilite::SourceHighlight *highlighter;
> +#endif

Should it be a unique_ptr so that valgrind doesn't complain about
it leaking when gdb exits?

> +
>  /* See source-cache.h.  */
>  
>  bool
> @@ -209,11 +218,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
>  		     use-after-free.  */
>  		  fullname = symtab_to_fullname (s);
>  		}
> -	      srchilite::SourceHighlight highlighter ("esc.outlang");
> -	      highlighter.setStyleFile("esc.style");
> +
> +	      if (highlighter == nullptr)
> +		{
> +		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
> +		  highlighter->setStyleFile("esc.style");

Preexisting, but missing space before parens.

> +		}

To keep the variable's definition and initialization close by,
I'd add a get_highlighter function:

 static std::unique_ptr<srchilite::SourceHighlight> highlighter;

 static srchilite::SourceHighlight *
 get_highlighter ()
 {
   if (highlighter == nullptr)
     {
       highlighter = new srchilite::SourceHighlight ("esc.outlang");
       highlighter->setStyleFile ("esc.style");
     }
 
   return highlighter.get ();
 }

>  
>  	      std::ostringstream output;
> -	      highlighter.highlight (input, output, lang_name, fullname);
> +	      highlighter->highlight (input, output, lang_name, fullname);
>  
>  	      source_text result = { fullname, output.str () };
>  	      m_source_map.push_back (std::move (result));

Thanks,
Pedro Alves

Re: [PATCH] Instantiate a single source highlighter

[Philippe Waroquiers]

On Tue, 2019-06-18 at 21:20 +0100, Pedro Alves wrote:
> > +/* The global source highlight object, or null if one was never
> > +   constructed.  This is stored here rather than in the class so that
> > +   we don't need to include anything or do conditional compilation in
> > +   source-cache.h.  */
> > +#ifdef HAVE_SOURCE_HIGHLIGHT
> > +static srchilite::SourceHighlight *highlighter;
> > +#endif
> 
> Should it be a unique_ptr so that valgrind doesn't complain about
> it leaking when gdb exits?
As highlighter is a global static variable,
when GDB exits, valgrind will consider the memory as reachable
and not a leak.

So, from only a valgrind point of view, there is no reason to have
GDB releasing this memory automatically when exiting.

Philippe

Re: [PATCH] Instantiate a single source highlighter

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +/* The global source highlight object, or null if one was never
>> +   constructed.  This is stored here rather than in the class so that
>> +   we don't need to include anything or do conditional compilation in
>> +   source-cache.h.  */
>> +#ifdef HAVE_SOURCE_HIGHLIGHT
>> +static srchilite::SourceHighlight *highlighter;
>> +#endif

Pedro> Should it be a unique_ptr so that valgrind doesn't complain about
Pedro> it leaking when gdb exits?

Philippe says no, so I didn't make this change.

>> +		  highlighter->setStyleFile("esc.style");

Pedro> Preexisting, but missing space before parens.

Fixed.

Pedro> To keep the variable's definition and initialization close by,
Pedro> I'd add a get_highlighter function:

I just moved the global to be a static in the block that uses it.
This seemed just as good and avoided more #ifdefs.

Here's what I'm checking in.

Tom

commit ca0c2edb7577c10c3772c8eed8e253971847ccdc
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Jun 18 12:18:24 2019 -0600

    Instantiate a single source highlighter
    
    It occurred to me that there's no reason to make a new source
    highlighter each time gdb needs to highlight some source code.
    Instead, a single one can be created and then simply reused each time.
    
    This patch implements this idea.  Tested on x86-64 Fedora 29.
    
    gdb/ChangeLog
    2019-06-19  Tom Tromey  <tromey@adacore.com>
    
            * source-cache.c (highlighter): New global.
            (source_cache::get_source_lines): Create a highlighter on demand.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 83f47b25701..1aab438d035 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-06-19  Tom Tromey  <tromey@adacore.com>
+
+	* source-cache.c (highlighter): New global.
+	(source_cache::get_source_lines): Create a highlighter on demand.
+
 2019-06-18  Tom de Vries  <tdevries@suse.de>
 
 	PR gdb/24515
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 2d5b549d971..86efe83bf9a 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -197,6 +197,13 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
 	  std::ifstream input (fullname);
 	  if (input.is_open ())
 	    {
+	      /* The global source highlight object, or null if one
+		 was never constructed.  This is stored here rather
+		 than in the class so that we don't need to include
+		 anything or do conditional compilation in
+		 source-cache.h.  */
+	      static srchilite::SourceHighlight *highlighter;
+
 	      if (s->line_charpos == 0)
 		{
 		  scoped_fd desc (open_source_file_with_line_charpos (s));
@@ -209,11 +216,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
 		     use-after-free.  */
 		  fullname = symtab_to_fullname (s);
 		}
-	      srchilite::SourceHighlight highlighter ("esc.outlang");
-	      highlighter.setStyleFile("esc.style");
+
+	      if (highlighter == nullptr)
+		{
+		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
+		  highlighter->setStyleFile ("esc.style");
+		}
 
 	      std::ostringstream output;
-	      highlighter.highlight (input, output, lang_name, fullname);
+	      highlighter->highlight (input, output, lang_name, fullname);
 
 	      source_text result = { fullname, output.str () };
 	      m_source_map.push_back (std::move (result));