| Message ID | 20230906220737.4049357-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: cache conversion function lookup | expand |
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
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 > >
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 --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"