diff mbox series

c++: cache conversion function lookup

Message ID 20230906220737.4049357-1-ppalka@redhat.com
State New
Headers
Series c++: cache conversion function lookup |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed

Commit Message

Patrick Palka Sept. 6, 2023, 10:07 p.m. UTC 
  Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?  This cache apparently has a 98% hit rate for TYPE_HAS_CONVERSION
types on some test files.

-- >8 --

gcc/cp/ChangeLog:

	* config-lang.in (gtfiles): Add search.cc.
	* search.cc (lookup_conversions_cache): Define.
	(lookup_conversions): Use it to cache he lookup.
	Improve cache rate by considering TYPE_MAIN_VARIANT
	of the type.
---
 gcc/cp/config-lang.in |  1 +
 gcc/cp/search.cc      | 11 +++++++++++
 2 files changed, 12 insertions(+)

Comments

Jason Merrill Sept. 7, 2023, 7:18 p.m. UTC | #1 
On 9/6/23 18:07, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?  This cache apparently has a 98% hit rate for TYPE_HAS_CONVERSION
> types on some test files.

Does it make a measurable difference in compile time?

> +/* A cache of the result of lookup_conversions.  */
> +
> +static GTY((cache)) type_tree_cache_map *lookup_conversions_cache;

This should probably be (deletable) rather than (cache)?

Jason
Patrick Palka Sept. 7, 2023, 8:12 p.m. UTC | #2 
On Thu, 7 Sep 2023, Jason Merrill wrote:

> On 9/6/23 18:07, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?  This cache apparently has a 98% hit rate for TYPE_HAS_CONVERSION
> > types on some test files.
> 
> Does it make a measurable difference in compile time?

Admittedly this optimization was probably more justified with the older
version of the PR99599 patch that added another lookup_conversions call.
Compile time for this standalone patch in the noise according to my
experiments, but I did measure a ~1MB/0.2% decrease in memory usage for
range-v3's zip.cpp.  This is because lookup_conversions_r allocates a
new list each time it's called (on a TYPE_HAS_CONVERSION type) even in
the simple case of no inheritance etc.  Maybe lookup_conversions_r's
(relatively complicated) implementation could be improved in the
simple/common case...

> 
> > +/* A cache of the result of lookup_conversions.  */
> > +
> > +static GTY((cache)) type_tree_cache_map *lookup_conversions_cache;
> 
> This should probably be (deletable) rather than (cache)?

Ack.  Is that because of PCH concerns or because the cache is
purely an optimization and so has no semantic effect?

> 
> Jason
> 
>
Jason Merrill Sept. 7, 2023, 9:34 p.m. UTC | #3 
On 9/7/23 16:12, Patrick Palka wrote:
> On Thu, 7 Sep 2023, Jason Merrill wrote:
> 
>> On 9/6/23 18:07, Patrick Palka wrote:
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>>> trunk?  This cache apparently has a 98% hit rate for TYPE_HAS_CONVERSION
>>> types on some test files.
>>
>> Does it make a measurable difference in compile time?
> 
> Admittedly this optimization was probably more justified with the older
> version of the PR99599 patch that added another lookup_conversions call.
> Compile time for this standalone patch in the noise according to my
> experiments, but I did measure a ~1MB/0.2% decrease in memory usage for
> range-v3's zip.cpp.  This is because lookup_conversions_r allocates a
> new list each time it's called (on a TYPE_HAS_CONVERSION type) even in
> the simple case of no inheritance etc.  Maybe lookup_conversions_r's
> (relatively complicated) implementation could be improved in the
> simple/common case...
> 
>>> +/* A cache of the result of lookup_conversions.  */
>>> +
>>> +static GTY((cache)) type_tree_cache_map *lookup_conversions_cache;
>>
>> This should probably be (deletable) rather than (cache)?
> 
> Ack.  Is that because of PCH concerns or because the cache is
> purely an optimization and so has no semantic effect?

The latter.  Really, (cache) is a terrible name, it should only be used 
when it's impractical to recompute the value.

Jason
diff mbox series

Patch

diff --git a/gcc/cp/config-lang.in b/gcc/cp/config-lang.in
index a6c7883cc24..e34c392d208 100644
--- a/gcc/cp/config-lang.in
+++ b/gcc/cp/config-lang.in
@@ -52,6 +52,7 @@  gtfiles="\
 \$(srcdir)/cp/name-lookup.cc \
 \$(srcdir)/cp/parser.cc \$(srcdir)/cp/pt.cc \
 \$(srcdir)/cp/rtti.cc \
+\$(srcdir)/cp/search.cc \
 \$(srcdir)/cp/semantics.cc \
 \$(srcdir)/cp/tree.cc \$(srcdir)/cp/typeck2.cc \
 \$(srcdir)/cp/vtable-class-hierarchy.cc \
diff --git a/gcc/cp/search.cc b/gcc/cp/search.cc
index cd80f285ac9..d5a2c57f59c 100644
--- a/gcc/cp/search.cc
+++ b/gcc/cp/search.cc
@@ -2582,6 +2582,10 @@  lookup_conversions_r (tree binfo, int virtual_depth, int virtualness,
   return my_virtualness;
 }
 
+/* A cache of the result of lookup_conversions.  */
+
+static GTY((cache)) type_tree_cache_map *lookup_conversions_cache;
+
 /* Return a TREE_LIST containing all the non-hidden user-defined
    conversion functions for TYPE (and its base-classes).  The
    TREE_VALUE of each node is the FUNCTION_DECL of the conversion
@@ -2596,10 +2600,14 @@  lookup_conversions (tree type)
 {
   tree convs;
 
+  type = TYPE_MAIN_VARIANT (type);
   complete_type (type);
   if (!CLASS_TYPE_P (type) || !TYPE_BINFO (type))
     return NULL_TREE;
 
+  if (tree *c = hash_map_safe_get (lookup_conversions_cache, type))
+    return *c;
+
   lookup_conversions_r (TYPE_BINFO (type), 0, 0, NULL_TREE, NULL_TREE, &convs);
 
   tree list = NULL_TREE;
@@ -2618,6 +2626,7 @@  lookup_conversions (tree type)
 	}
     }
 
+  hash_map_safe_put<hm_ggc> (lookup_conversions_cache, type, list);
   return list;
 }
 
@@ -2798,3 +2807,5 @@  any_dependent_bases_p (tree type)
 
   return false;
 }
+
+#include "gt-cp-search.h"