Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
[adding list back]
On Apr 18, 2018, Jakub Jelinek wrote:
> On Wed, Apr 18, 2018 at 12:29:03AM -0300, Alexandre Oliva wrote:
>> + static void free(tinst_level *obj);
> ^
>+ missing space
Thanks, fixed with this patch:
diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog
index 7cd714b9e3ad..4475b228c2eb 100644
--- a/gcc/cp/ChangeLog
+++ b/gcc/cp/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-19 Alexandre Oliva
+
+ PR c++/80290
+ * cp-tree.h (tinst_level::free): Fix whitespace.
+
2018-04-18 Paolo Carlini
PR c++/84630
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 7031c79b35db..8c5c84e22b22 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5904,7 +5904,7 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
public:
/* Release storage for OBJ and node, if it's a TREE_LIST. */
- static void free(tinst_level *obj);
+ static void free (tinst_level *obj);
/* Return TRUE iff the original node is a list, split or not. */
bool list_p () const { return !not_list_p (); }
--
Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Apr 18, 2018, Jason Merrill wrote: > Ok, thanks. Thanks, here's the consolidated patch I installed. tinst_level objects are created during template instantiation, and they're most often quite short-lived, but since there's no intervening garbage collection, they accumulate throughout the pass while most by far could be recycled after a short while. The original testcase in PR80290, for example, creates almost 55 million tinst_level objects, all but 10 thousand of them without intervening GC, but we don't need more than 284 of them live at a time. Furthermore, in many cases, TREE_LIST objects are created to stand for the decl in tinst_level. In most cases, those can be recycled as soon as the tinst_level object is recycled; in some relatively common cases, they are modified and reused in up to 3 tinst_level objects. In the same testcase, TREE_LISTs are used in all but 3 thousand of the tinst_level objects, and we don't need more than 89 tinst_level objects with TREE_LISTs live at a time. Furthermore, all but 2 thousand of those are created without intervening GC. This patch arranges for tinst_level objects to be refcounted (I've seen as many as 20 live references to a single tinst_level object in my testing), and for pending_template, tinst_level and TREE_LIST objects that can be recycled to be added to freelists; that's much faster than ggc_free()ing them and reallocating them shortly thereafter. In fact, the introduction of such freelists is what makes this entire patch lighter-weight, when it comes to memory use, and faster. With refcounting alone, the total memory footprint was still about the same, and we spent more time. In order to further reduce memory use, I've arranged for us to create TREE_LISTs lazily, only at points that really need them (when printing error messages). This grows tinst_level by an additional pointer, but since a TREE_LIST holds a lot more than an extra pointer, and tinst_levels with TREE_LISTs used to be allocated tens of thousands of times more often than plain decl ones, we still save memory overall. I was still not quite happy about growing memory use in cases that used template classes but not template overload resolution, so I changed the type of the errors field to unsigned short, from int. With that change, in_system_header_p and refcount move into the same word or half-word that used to hold errors, releasing a full word, bringing tinst_level back to its original size, now without any padding. The errors field is only used to test whether we've had any errors since the expansion of some template, to skip the expansion of further templates. If we get 2^16 errors or more, it is probably reasonable to refrain from expanding further templates, even if we would expand them before this change. With these changes, compile time for the original testcase at -O0, with default checking enabled, is cut down by some 3.7%, total GCable memory allocation is cut down by almost 20%, and total memory use (as reported by GNU time as maxresident) is cut down by slightly over 15%. for gcc/cp/ChangeLog PR c++/80290 * cp-tree.h (struct tinst_level): Split decl into tldcl and targs. Add private split_list_p, tree_list_p, and not_list_p inline const predicates; to_list private member function declaration; free public member function declaration; list_p, get_node and maybe_get_node accessors, and refcount data member. Narrow errors to unsigned short. * error.c (print_instantiation_full_context): Use new accessors. (print_instantiation_partial_context_line): Likewise. Drop const from tinst_level-typed parameter. * mangle.c (mangle_decl_string): Likewise. * pt.c (freelist): New template class. (tree_list_freelist_head): New var. (tree_list_freelist): New fn, along with specializations. (tinst_level_freelist_head): New var. (pending_template_freelist_head): Likewise. (tinst_level_freelist, pending_template_freelist): New fns. (tinst_level::to_list, tinst_level::free): Define. (inc_refcount_use, dec_refcount_use): New fns for tinst_level. (set_refcount_ptr): New template fn. (add_pending_template): Adjust for refcounting, freelists and new accessors. (neglectable_inst_p): Take a NULL d as a non-DECL. (limit_bad_template_recursion): Use new accessors. (push_tinst_level): New overload to create the list. (push_tinst_level_loc): Make it static, split decl into two args, adjust tests and initialization to cope with split lists, use freelist, adjust for refcounting. (push_tinst_level_loc): New wrapper with the old interface. (pop_tinst_level): Adjust for refcounting. (record_last_problematic_instantiation): Likewise. (reopen_tinst_level): Likewise. Use new accessors. (instantiate_alias_template): Adjust for s
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
Ok, thanks.
On Tue, Apr 17, 2018, 9:29 PM Alexandre Oliva wrote:
> On Apr 17, 2018, Jason Merrill wrote:
>
> >> Any objections to making dec_refcount_use a friend of tinst_level's?
> >> Otherwise, I'd rather add a free() member function (or maybe static
> >> member function) to free both the TREE_LIST and the tinst_level objects.
>
> > Either of those sounds fine.
>
> Here's the additional incremental patch I'm testing.
>
> I've added a number of line breaks before opening braces in function
> definitions, that had escaped my attention in the initial patch
> submission.
>
> ---
> gcc/cp/cp-tree.h |5 -
> gcc/cp/mangle.c |2 +-
> gcc/cp/pt.c | 56
> --
> 3 files changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 26a50ac136dd..7031c79b35db 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5903,6 +5903,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
>tree to_list ();
>
> public:
> + /* Release storage for OBJ and node, if it's a TREE_LIST. */
> + static void free(tinst_level *obj);
> +
>/* Return TRUE iff the original node is a list, split or not. */
>bool list_p () const { return !not_list_p (); }
>
> @@ -5916,7 +5919,7 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
>
>/* Return the original node if it's a DECL or a TREE_LIST, but do
> NOT convert a split list to a TREE_LIST: return NULL instead. */
> - tree get_decl_maybe () const {
> + tree maybe_get_node () const {
> if (!split_list_p ()) return tldcl;
> else return NULL_TREE;
>}
> diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
> index 940f7ed87e20..2f65709d7d8c 100644
> --- a/gcc/cp/mangle.c
> +++ b/gcc/cp/mangle.c
> @@ -3777,7 +3777,7 @@ mangle_decl_string (const tree decl)
>if (DECL_LANG_SPECIFIC (decl) && DECL_USE_TEMPLATE (decl))
> {
>struct tinst_level *tl = current_instantiation ();
> - if ((!tl || tl->get_decl_maybe () != decl)
> + if ((!tl || tl->maybe_get_node () != decl)
> && push_tinst_level (decl))
> {
> template_p = true;
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 2442f07095ca..79563dfa5334 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -50,7 +50,8 @@ typedef int (*tree_fn_t) (tree, void*);
> /* The PENDING_TEMPLATES is a TREE_LIST of templates whose
> instantiations have been deferred, either because their definitions
> were not yet available, or because we were putting off doing the
> work. */
> -struct GTY ((chain_next ("%h.next"))) pending_template {
> +struct GTY ((chain_next ("%h.next"))) pending_template
> +{
>struct pending_template *next;
>struct tinst_level *tinst;
> };
> @@ -8839,17 +8840,20 @@ public:
> build_tree_list logic in reinit, so this could go out of sync. */
> template <>
> inline tree &
> -freelist::next (tree obj) {
> +freelist::next (tree obj)
> +{
>return TREE_CHAIN (obj);
> }
> template <>
> inline tree
> -freelist::anew () {
> +freelist::anew ()
> +{
>return build_tree_list (NULL, NULL);
> }
> template <>
> inline void
> -freelist::poison (tree obj ATTRIBUTE_UNUSED) {
> +freelist::poison (tree obj ATTRIBUTE_UNUSED)
> +{
>int size ATTRIBUTE_UNUSED = sizeof (tree_list);
>tree p ATTRIBUTE_UNUSED = obj;
>tree_base *b ATTRIBUTE_UNUSED = &obj->base;
> @@ -8878,7 +8882,8 @@ freelist::poison (tree obj
> ATTRIBUTE_UNUSED) {
> }
> template <>
> inline void
> -freelist::reinit (tree obj ATTRIBUTE_UNUSED) {
> +freelist::reinit (tree obj ATTRIBUTE_UNUSED)
> +{
>tree_base *b ATTRIBUTE_UNUSED = &obj->base;
>
> #ifdef ENABLE_GC_CHECKING
> @@ -8902,7 +8907,8 @@ freelist::reinit (tree obj
> ATTRIBUTE_UNUSED) {
> static GTY((deletable)) tree tree_list_freelist_head;
> /* Return the/an actual TREE_LIST freelist. */
> static inline freelist
> -tree_list_freelist () {
> +tree_list_freelist ()
> +{
>return tree_list_freelist_head;
> }
>
> @@ -8910,7 +8916,8 @@ tree_list_freelist () {
> static GTY((deletable)) tinst_level *tinst_level_freelist_head;
> /* Return the/an actual tinst_level freelist. */
> static inline freelist
> -tinst_level_freelist () {
> +tinst_level_freelist ()
> +{
>return tinst_level_freelist_head;
> }
>
> @@ -8918,7 +8925,8 @@ tinst_level_freelist () {
> static GTY((deletable)) pending_template *pending_template_freelist_head;
> /* Return the/an actual pending_template freelist. */
> static inline freelist
> -pending_template_freelist () {
> +pending_template_freelist ()
> +{
>return pending_template_freelist_head;
> }
>
> @@ -8939,7 +8947,8 @@ tinst_level::to_list ()
>
> /* Increment OBJ's refcount. */
> static tinst_level *
> -inc_refcount_use (tinst_level *obj) {
> +inc_refcount_use (tinst_level *obj)
> +{
>if (obj)
> {
>++obj->refcount;
> @@ -8948,18 +8957,26 @@ inc_refcount_use (tinst_level *obj) {
>return obj;
> }
>
> +/* Release storage
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Apr 17, 2018, Jason Merrill wrote:
>> Any objections to making dec_refcount_use a friend of tinst_level's?
>> Otherwise, I'd rather add a free() member function (or maybe static
>> member function) to free both the TREE_LIST and the tinst_level objects.
> Either of those sounds fine.
Here's the additional incremental patch I'm testing.
I've added a number of line breaks before opening braces in function
definitions, that had escaped my attention in the initial patch
submission.
---
gcc/cp/cp-tree.h |5 -
gcc/cp/mangle.c |2 +-
gcc/cp/pt.c | 56 --
3 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 26a50ac136dd..7031c79b35db 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5903,6 +5903,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
tree to_list ();
public:
+ /* Release storage for OBJ and node, if it's a TREE_LIST. */
+ static void free(tinst_level *obj);
+
/* Return TRUE iff the original node is a list, split or not. */
bool list_p () const { return !not_list_p (); }
@@ -5916,7 +5919,7 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
/* Return the original node if it's a DECL or a TREE_LIST, but do
NOT convert a split list to a TREE_LIST: return NULL instead. */
- tree get_decl_maybe () const {
+ tree maybe_get_node () const {
if (!split_list_p ()) return tldcl;
else return NULL_TREE;
}
diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index 940f7ed87e20..2f65709d7d8c 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -3777,7 +3777,7 @@ mangle_decl_string (const tree decl)
if (DECL_LANG_SPECIFIC (decl) && DECL_USE_TEMPLATE (decl))
{
struct tinst_level *tl = current_instantiation ();
- if ((!tl || tl->get_decl_maybe () != decl)
+ if ((!tl || tl->maybe_get_node () != decl)
&& push_tinst_level (decl))
{
template_p = true;
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 2442f07095ca..79563dfa5334 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -50,7 +50,8 @@ typedef int (*tree_fn_t) (tree, void*);
/* The PENDING_TEMPLATES is a TREE_LIST of templates whose
instantiations have been deferred, either because their definitions
were not yet available, or because we were putting off doing the work. */
-struct GTY ((chain_next ("%h.next"))) pending_template {
+struct GTY ((chain_next ("%h.next"))) pending_template
+{
struct pending_template *next;
struct tinst_level *tinst;
};
@@ -8839,17 +8840,20 @@ public:
build_tree_list logic in reinit, so this could go out of sync. */
template <>
inline tree &
-freelist::next (tree obj) {
+freelist::next (tree obj)
+{
return TREE_CHAIN (obj);
}
template <>
inline tree
-freelist::anew () {
+freelist::anew ()
+{
return build_tree_list (NULL, NULL);
}
template <>
inline void
-freelist::poison (tree obj ATTRIBUTE_UNUSED) {
+freelist::poison (tree obj ATTRIBUTE_UNUSED)
+{
int size ATTRIBUTE_UNUSED = sizeof (tree_list);
tree p ATTRIBUTE_UNUSED = obj;
tree_base *b ATTRIBUTE_UNUSED = &obj->base;
@@ -8878,7 +8882,8 @@ freelist::poison (tree obj ATTRIBUTE_UNUSED) {
}
template <>
inline void
-freelist::reinit (tree obj ATTRIBUTE_UNUSED) {
+freelist::reinit (tree obj ATTRIBUTE_UNUSED)
+{
tree_base *b ATTRIBUTE_UNUSED = &obj->base;
#ifdef ENABLE_GC_CHECKING
@@ -8902,7 +8907,8 @@ freelist::reinit (tree obj ATTRIBUTE_UNUSED) {
static GTY((deletable)) tree tree_list_freelist_head;
/* Return the/an actual TREE_LIST freelist. */
static inline freelist
-tree_list_freelist () {
+tree_list_freelist ()
+{
return tree_list_freelist_head;
}
@@ -8910,7 +8916,8 @@ tree_list_freelist () {
static GTY((deletable)) tinst_level *tinst_level_freelist_head;
/* Return the/an actual tinst_level freelist. */
static inline freelist
-tinst_level_freelist () {
+tinst_level_freelist ()
+{
return tinst_level_freelist_head;
}
@@ -8918,7 +8925,8 @@ tinst_level_freelist () {
static GTY((deletable)) pending_template *pending_template_freelist_head;
/* Return the/an actual pending_template freelist. */
static inline freelist
-pending_template_freelist () {
+pending_template_freelist ()
+{
return pending_template_freelist_head;
}
@@ -8939,7 +8947,8 @@ tinst_level::to_list ()
/* Increment OBJ's refcount. */
static tinst_level *
-inc_refcount_use (tinst_level *obj) {
+inc_refcount_use (tinst_level *obj)
+{
if (obj)
{
++obj->refcount;
@@ -8948,18 +8957,26 @@ inc_refcount_use (tinst_level *obj) {
return obj;
}
+/* Release storage for OBJ and node, if it's a TREE_LIST. */
+void
+tinst_level::free (tinst_level *obj)
+{
+ if (obj->tree_list_p ())
+tree_list_freelist ().free (obj->get_node ());
+ tinst_level_freelist ().free (obj);
+}
+
/* Decrement OBJ's refcount. If it reaches zero, release OBJ's DECL
and OBJ, and start over with the tinst_level object that used
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Tue, Apr 17, 2018, 10:25 AM Alexandre Oliva wrote: > On Apr 17, 2018, Jason Merrill wrote: > > > I was thinking maybe_get_node > > 'k > > > I suppose better would be for > > > + if (obj->list_p () && obj->get_decl_maybe ()) > > + tree_list_freelist ().free (obj->get_node ()); > > > to be e.g. > > > obj-> maybe_free_list(); > > Any objections to making dec_refcount_use a friend of tinst_level's? > Otherwise, I'd rather add a free() member function (or maybe static > member function) to free both the TREE_LIST and the tinst_level objects. > Either of those sounds fine. Otherwise, maybe_free_list would leave the object in an inconsistent > state, pointing to a freed TREE_LIST, or it could undo to_list, though > it's not supposed to be reversible in the lifetime of the object. > free()ing the entire object along with the TREE_LIST addresses that > concern. But then, dec_refcount_use does that and then some... > > > Or put the list handling in a destructor for tinst_level, and have > > freelist use placement new and explicit destruction. > > A destructor would enable finalization in GGC, I don't think we want to > do that. > > -- > Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer >
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Apr 17, 2018, Jason Merrill wrote: > I was thinking maybe_get_node 'k > I suppose better would be for > + if (obj->list_p () && obj->get_decl_maybe ()) > + tree_list_freelist ().free (obj->get_node ()); > to be e.g. > obj-> maybe_free_list(); Any objections to making dec_refcount_use a friend of tinst_level's? Otherwise, I'd rather add a free() member function (or maybe static member function) to free both the TREE_LIST and the tinst_level objects. Otherwise, maybe_free_list would leave the object in an inconsistent state, pointing to a freed TREE_LIST, or it could undo to_list, though it's not supposed to be reversible in the lifetime of the object. free()ing the entire object along with the TREE_LIST addresses that concern. But then, dec_refcount_use does that and then some... > Or put the list handling in a destructor for tinst_level, and have > freelist use placement new and explicit destruction. A destructor would enable finalization in GGC, I don't think we want to do that. -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Mon, Apr 16, 2018 at 9:45 PM, Alexandre Oliva wrote: > On Apr 16, 2018, Jason Merrill wrote: > >> This change looks good. One other nit: get_decl_maybe can also return a >> list, so the name seems wrong. > > Uhh, it's "give me the decl if you got one, but it's ok if you give me a > list" (the latter is what makes it _maybe). It replaces what used to be > called just 'decl'. Maybe wrong is a bit too strong, but... do you > have any suggestion? get_decl_or_tree_list_but_dont_build_the_list is a > bit too long IMHO ;-) I was thinking maybe_get_node, since it returns either the same as get_node or null (and maybe usually comes early in a function name). >> And this line > >> + if (obj->list_p () && obj->get_decl_maybe ()) > >> could be > >> if (tree_list_p ()) > > It could if we made tree_list_p public. It's private because that's an > internal implementation detail, though it's one that happens to leak > through the combination of calls above. I suppose better would be for + if (obj->list_p () && obj->get_decl_maybe ()) + tree_list_freelist ().free (obj->get_node ()); to be e.g. obj->maybe_free_list(); Or put the list handling in a destructor for tinst_level, and have freelist use placement new and explicit destruction. Jason
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Apr 16, 2018, Jason Merrill wrote: > This change looks good. One other nit: get_decl_maybe can also return a > list, so the name seems wrong. Uhh, it's "give me the decl if you got one, but it's ok if you give me a list" (the latter is what makes it _maybe). It replaces what used to be called just 'decl'. Maybe wrong is a bit too strong, but... do you have any suggestion? get_decl_or_tree_list_but_dont_build_the_list is a bit too long IMHO ;-) > And this line > + if (obj->list_p () && obj->get_decl_maybe ()) > could be > if (tree_list_p ()) It could if we made tree_list_p public. It's private because that's an internal implementation detail, though it's one that happens to leak through the combination of calls above. > I think, and then get_decl_maybe wouldn't need to return a list anymore? That's backwards. It doesn't need to; the point of get_decl_maybe is to make as simple a test as possible and return fast, without having to look in the "decl" to find out what it is. I even considered making it return tldcl unconditionally, but that would just cause trouble for the various pieces of code that used to compare tinst_level::decls for equality, and would now get false positives out of split lists sharing the same tmpl decl in the first element of the list. Testing targs in get_decl_maybe to avoid returning a partial former decl rules out this case and is as cheap as it gets. Do we need more detailed comments, besides a name change? -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Mon, Apr 16, 2018, 3:20 PM Alexandre Oliva wrote:
> On Apr 16, 2018, Jason Merrill wrote:
>
> > On Fri, Apr 13, 2018, 5:19 PM Alexandre Oliva wrote:
> >> + tree get_node () const {
> >> +if (!split_list_p ()) return tldcl;
> >> +else return const_cast (this)->to_list ();
> >> + }
>
> > This looks like a dangerous violation of const correctness, since it does
> > in fact modify the object.
>
> Well... We know the object's actual type is not const, since we
> allocate all objects of this type from GCable memory. Conceptually, the
> object is not modified, we're just lazily performing an expensive
> rearrangement of the internal representation that we would have done
> upfront if we weren't trying to save that expense. This would be a
> perfect case for mutable, if only gengtype didn't get confused by it.
>
>
> OTOH, we could drop a tiny amount of constification in error.c and avoid
> the problem altogether, as in the following incremental patch:
>
> gcc/cp/cp-tree.h |4 ++--
> gcc/cp/error.c |2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index e9d9bab879bc..26a50ac136dd 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -5909,9 +5909,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
>/* Return the original node; if it's a split list, make it a
> TREE_LIST first, so that it can be returned as a single tree
> object. */
> - tree get_node () const {
> + tree get_node () {
> if (!split_list_p ()) return tldcl;
> -else return const_cast (this)->to_list ();
> +else return to_list ();
>}
>
>/* Return the original node if it's a DECL or a TREE_LIST, but do
> diff --git a/gcc/cp/error.c b/gcc/cp/error.c
> index 983ffdfedbb2..95b8b84f34ab 100644
> --- a/gcc/cp/error.c
> +++ b/gcc/cp/error.c
> @@ -3475,7 +3475,7 @@ print_instantiation_full_context (diagnostic_context
> *context)
>
> static void
> print_instantiation_partial_context_line (diagnostic_context *context,
> - const struct tinst_level *t,
> + struct tinst_level *t,
> location_t loc, bool recursive_p)
> {
>if (loc == UNKNOWN_LOCATION)
>
>
> Does this incremental change make it ok (pending retesting), or should I
> look into adding mutable support to gengtype, or something else?
>
This change looks good. One other nit: get_decl_maybe can also return a
list, so the name seems wrong. And this line
+ if (obj->list_p () && obj->get_decl_maybe ())
could be
if (tree_list_p ())
I think, and then get_decl_maybe wouldn't need to return a list anymore?
Jason
--
> Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/ FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
>
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Apr 16, 2018, Jason Merrill wrote:
> On Fri, Apr 13, 2018, 5:19 PM Alexandre Oliva wrote:
>> + tree get_node () const {
>> +if (!split_list_p ()) return tldcl;
>> +else return const_cast (this)->to_list ();
>> + }
> This looks like a dangerous violation of const correctness, since it does
> in fact modify the object.
Well... We know the object's actual type is not const, since we
allocate all objects of this type from GCable memory. Conceptually, the
object is not modified, we're just lazily performing an expensive
rearrangement of the internal representation that we would have done
upfront if we weren't trying to save that expense. This would be a
perfect case for mutable, if only gengtype didn't get confused by it.
OTOH, we could drop a tiny amount of constification in error.c and avoid
the problem altogether, as in the following incremental patch:
gcc/cp/cp-tree.h |4 ++--
gcc/cp/error.c |2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index e9d9bab879bc..26a50ac136dd 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5909,9 +5909,9 @@ struct GTY((chain_next ("%h.next"))) tinst_level {
/* Return the original node; if it's a split list, make it a
TREE_LIST first, so that it can be returned as a single tree
object. */
- tree get_node () const {
+ tree get_node () {
if (!split_list_p ()) return tldcl;
-else return const_cast (this)->to_list ();
+else return to_list ();
}
/* Return the original node if it's a DECL or a TREE_LIST, but do
diff --git a/gcc/cp/error.c b/gcc/cp/error.c
index 983ffdfedbb2..95b8b84f34ab 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -3475,7 +3475,7 @@ print_instantiation_full_context (diagnostic_context
*context)
static void
print_instantiation_partial_context_line (diagnostic_context *context,
- const struct tinst_level *t,
+ struct tinst_level *t,
location_t loc, bool recursive_p)
{
if (loc == UNKNOWN_LOCATION)
Does this incremental change make it ok (pending retesting), or should I
look into adding mutable support to gengtype, or something else?
--
Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
Re: [PATCH] [PR c++/80290] recycle tinst garbage sooner
On Fri, Apr 13, 2018, 5:19 PM Alexandre Oliva wrote: > tinst_level objects are created during template instantiation, and > they're most often quite short-lived, but since there's no intervening > garbage collection, they accumulate throughout the pass while most by > far could be recycled after a short while. The original testcase in > PR80290, for example, creates almost 55 million tinst_level objects, > all but 10 thousand of them without intervening GC, but we don't need > more than 284 of them live at a time. > > Furthermore, in many cases, TREE_LIST objects are created to stand for > the decl in tinst_level. In most cases, those can be recycled as soon > as the tinst_level object is recycled; in some relatively common > cases, they are modified and reused in up to 3 tinst_level objects. > In the same testcase, TREE_LISTs are used in all but 3 thousand of the > tinst_level objects, and we don't need more than 89 tinst_level > objects with TREE_LISTs live at a time. Furthermore, all but 2 > thousand of those are created without intervening GC. > > This patch arranges for tinst_level objects to be refcounted (I've > seen as many as 20 live references to a single tinst_level object in > my testing), and for pending_template, tinst_level and TREE_LIST > objects that can be recycled to be added to freelists; that's much > faster than ggc_free()ing them and reallocating them shortly > thereafter. In fact, the introduction of such freelists is what makes > this entire patch lighter-weight, when it comes to memory use, and > faster. With refcounting alone, the total memory footprint was still > about the same, and we spent more time. > > In order to further reduce memory use, I've arranged for us to create > TREE_LISTs lazily, only at points that really need them (when printing > error messages). This grows tinst_level by an additional pointer, but > since a TREE_LIST holds a lot more than an extra pointer, and > tinst_levels with TREE_LISTs used to be allocated tens of thousands of > times more often than plain decl ones, we still save memory overall. > > I was still not quite happy about growing memory use in cases that > used template classes but not template overload resolution, so I > changed the type of the errors field to unsigned short, from int. > With that change, in_system_header_p and refcount move into the same > word or half-word that used to hold errors, releasing a full word, > bringing tinst_level back to its original size, now without any > padding. > > The errors field is only used to test whether we've had any errors > since the expansion of some template, to skip the expansion of further > templates. If we get 2^16 errors or more, it is probably reasonable > to refrain from expanding further templates, even if we would expand > them before this change. > > With these changes, compile time for the original testcase at -O0, > with default checking enabled, is cut down by some 3.7%, total GCable > memory allocation is cut down by almost 20%, and total memory use (as > reported by GNU time as maxresident) is cut down by slightly over 15%. > Great! Regstrapped on i686- and x86_64-linux-gnu. Ok to install? > (See below for an incremental patch that introduces some statistics > collection and reporting, that shows how I collected some of the data > above. That one is not meant for inclusion) > > for gcc/cp/ChangeLog > > PR c++/80290 > * cp-tree.h (struct tinst_level): Split decl into tldcl and > targs. Add private split_list_p, tree_list_p, and not_list_p > inline const predicates; to_list private member function > declaration; list_p, get_node, and get_decl_maybe accessors, > and refcount private data member. Narrow errors to unsigned > short. > * error.c (print_instantiation_full_context): Use new > accessors. > (print_instantiation_partial_context_line): Likewise. > * mangle.c (mangle_decl_string): Likewise. > * pt.c (freelist): New template class. > (tree_list_freelist_head): New var. > (tree_list_freelist): New fn, along with specializations. > (tinst_level_freelist_head): New var. > (pending_template_freelist_head): Likewise. > (tinst_level_freelist, pending_template_freelist): New fns. > (tinst_level::to_list): Define. > (inc_refcount_use, dec_refcount_use): New fns for tinst_level. > (set_refcount_ptr): New template fn. > (add_pending_template): Adjust for refcounting, freelists and > new accessors. > (neglectable_inst_p): Take a NULL d as a non-DECL. > (limit_bad_template_recursion): Use new accessors. > (push_tinst_level): New overload to create the list. > (push_tinst_level_loc): Make it static, split decl into two > args, adjust tests and initialization to cope with split > lists, use freelist, adjust for refcounting. > (push_tinst_level_lo
[PATCH] [PR c++/80290] recycle tinst garbage sooner
tinst_level objects are created during template instantiation, and they're most often quite short-lived, but since there's no intervening garbage collection, they accumulate throughout the pass while most by far could be recycled after a short while. The original testcase in PR80290, for example, creates almost 55 million tinst_level objects, all but 10 thousand of them without intervening GC, but we don't need more than 284 of them live at a time. Furthermore, in many cases, TREE_LIST objects are created to stand for the decl in tinst_level. In most cases, those can be recycled as soon as the tinst_level object is recycled; in some relatively common cases, they are modified and reused in up to 3 tinst_level objects. In the same testcase, TREE_LISTs are used in all but 3 thousand of the tinst_level objects, and we don't need more than 89 tinst_level objects with TREE_LISTs live at a time. Furthermore, all but 2 thousand of those are created without intervening GC. This patch arranges for tinst_level objects to be refcounted (I've seen as many as 20 live references to a single tinst_level object in my testing), and for pending_template, tinst_level and TREE_LIST objects that can be recycled to be added to freelists; that's much faster than ggc_free()ing them and reallocating them shortly thereafter. In fact, the introduction of such freelists is what makes this entire patch lighter-weight, when it comes to memory use, and faster. With refcounting alone, the total memory footprint was still about the same, and we spent more time. In order to further reduce memory use, I've arranged for us to create TREE_LISTs lazily, only at points that really need them (when printing error messages). This grows tinst_level by an additional pointer, but since a TREE_LIST holds a lot more than an extra pointer, and tinst_levels with TREE_LISTs used to be allocated tens of thousands of times more often than plain decl ones, we still save memory overall. I was still not quite happy about growing memory use in cases that used template classes but not template overload resolution, so I changed the type of the errors field to unsigned short, from int. With that change, in_system_header_p and refcount move into the same word or half-word that used to hold errors, releasing a full word, bringing tinst_level back to its original size, now without any padding. The errors field is only used to test whether we've had any errors since the expansion of some template, to skip the expansion of further templates. If we get 2^16 errors or more, it is probably reasonable to refrain from expanding further templates, even if we would expand them before this change. With these changes, compile time for the original testcase at -O0, with default checking enabled, is cut down by some 3.7%, total GCable memory allocation is cut down by almost 20%, and total memory use (as reported by GNU time as maxresident) is cut down by slightly over 15%. Regstrapped on i686- and x86_64-linux-gnu. Ok to install? (See below for an incremental patch that introduces some statistics collection and reporting, that shows how I collected some of the data above. That one is not meant for inclusion) for gcc/cp/ChangeLog PR c++/80290 * cp-tree.h (struct tinst_level): Split decl into tldcl and targs. Add private split_list_p, tree_list_p, and not_list_p inline const predicates; to_list private member function declaration; list_p, get_node, and get_decl_maybe accessors, and refcount private data member. Narrow errors to unsigned short. * error.c (print_instantiation_full_context): Use new accessors. (print_instantiation_partial_context_line): Likewise. * mangle.c (mangle_decl_string): Likewise. * pt.c (freelist): New template class. (tree_list_freelist_head): New var. (tree_list_freelist): New fn, along with specializations. (tinst_level_freelist_head): New var. (pending_template_freelist_head): Likewise. (tinst_level_freelist, pending_template_freelist): New fns. (tinst_level::to_list): Define. (inc_refcount_use, dec_refcount_use): New fns for tinst_level. (set_refcount_ptr): New template fn. (add_pending_template): Adjust for refcounting, freelists and new accessors. (neglectable_inst_p): Take a NULL d as a non-DECL. (limit_bad_template_recursion): Use new accessors. (push_tinst_level): New overload to create the list. (push_tinst_level_loc): Make it static, split decl into two args, adjust tests and initialization to cope with split lists, use freelist, adjust for refcounting. (push_tinst_level_loc): New wrapper with the old interface. (pop_tinst_level): Adjust for refcounting. (record_last_problematic_instantiation): Likewise. (reopen_tinst_level): Likewise. Use new accessors. (instantiate_
