| Message ID | 20151124084538.GB90197@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 24, 2015 at 9:45 AM, Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > while looking into the earlier bug I noticed that type_hash_canon is used to > avoid producing type duplicates. The type is however created, then looked up > in the cache and if it exists it is never used. The function takes care to > remove the code found memory stats but never actually free it. I suppose it is > artifact of conversion from obstack to ggc. We produce a lot of type duplicates > this way that is not very cool. > > It always bit bothered me that unify_scc does not update the statistics so > this patch introduces free_node that can be used to free given tree node. > > Bootstrapped/regtested x86_64-linux, OK? Ok. Thanks, Richard. > > * tree.c (free_node): New function. > (type_hash_canon): Use it. > * tree.h (free_node): Declare. > * lto.c (unify_scc): Use free_node. > > Index: tree.c > =================================================================== > --- tree.c (revision 230718) > +++ tree.c (working copy) > @@ -1103,6 +1103,27 @@ make_node_stat (enum tree_code code MEM_ > > return t; > } > + > +/* Free tree node. */ > + > +void > +free_node (tree node) > +{ > + enum tree_code code = TREE_CODE (node); > + if (GATHER_STATISTICS) > + { > + tree_code_counts[(int) TREE_CODE (node)]--; > + tree_node_counts[(int) t_kind]--; > + tree_node_sizes[(int) t_kind] -= tree_code_size (TREE_CODE (node)); > + } > + if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) > + vec_free (CONSTRUCTOR_ELTS (node)); > + else if (code == BLOCK) > + vec_free (BLOCK_NONLOCALIZED_VARS (node)); > + else if (code == TREE_BINFO) > + vec_free (BINFO_BASE_ACCESSES (node)); > + ggc_free (node); > +} > > /* Return a new node with the same contents as NODE except that its > TREE_CHAIN, if it has one, is zero and it has a fresh uid. */ > @@ -7100,12 +7123,7 @@ type_hash_canon (unsigned int hashcode, > { > tree t1 = ((type_hash *) *loc)->type; > gcc_assert (TYPE_MAIN_VARIANT (t1) == t1); > - if (GATHER_STATISTICS) > - { > - tree_code_counts[(int) TREE_CODE (type)]--; > - tree_node_counts[(int) t_kind]--; > - tree_node_sizes[(int) t_kind] -= sizeof (struct tree_type_non_common); > - } > + free_node (type); > return t1; > } > else > Index: tree.h > =================================================================== > --- tree.h (revision 230718) > +++ tree.h (working copy) > @@ -3763,6 +3763,10 @@ extern int allocate_decl_uid (void); > extern tree make_node_stat (enum tree_code MEM_STAT_DECL); > #define make_node(t) make_node_stat (t MEM_STAT_INFO) > > +/* Free tree node. */ > + > +extern void free_node (tree); > + > /* Make a copy of a node, with all the same contents. */ > > extern tree copy_node_stat (tree MEM_STAT_DECL); > Index: lto/lto.c > =================================================================== > --- lto/lto.c (revision 230718) > +++ lto/lto.c (working copy) > @@ -1623,13 +1622,9 @@ unify_scc (struct data_in *data_in, unsi > data_in->location_cache.revert_location_cache (); > for (unsigned i = 0; i < len; ++i) > { > - enum tree_code code; > if (TYPE_P (scc->entries[i])) > num_merged_types++; > - code = TREE_CODE (scc->entries[i]); > - if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) > - vec_free (CONSTRUCTOR_ELTS (scc->entries[i])); > - ggc_free (scc->entries[i]); > + free_node (scc->entries[i]); > } > > break;
On November 24, 2015 10:59:01 AM GMT+01:00, Richard Biener <richard.guenther@gmail.com> wrote: >On Tue, Nov 24, 2015 at 9:45 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >> Hi, >> while looking into the earlier bug I noticed that type_hash_canon is >used to >> avoid producing type duplicates. The type is however created, then >looked up >> in the cache and if it exists it is never used. The function takes >care to >> remove the code found memory stats but never actually free it. I >suppose it is >> artifact of conversion from obstack to ggc. We produce a lot of type >duplicates >> this way that is not very cool. >> >> It always bit bothered me that unify_scc does not update the >statistics so >> this patch introduces free_node that can be used to free given tree >node. >> >> Bootstrapped/regtested x86_64-linux, OK? > >Ok. ISTM that you could have used code also for the statistics. Thanks, > >Thanks, >Richard. > >> >> * tree.c (free_node): New function. >> (type_hash_canon): Use it. >> * tree.h (free_node): Declare. >> * lto.c (unify_scc): Use free_node. >> >> Index: tree.c >> =================================================================== >> --- tree.c (revision 230718) >> +++ tree.c (working copy) >> @@ -1103,6 +1103,27 @@ make_node_stat (enum tree_code code MEM_ >> >> return t; >> } >> + >> +/* Free tree node. */ >> + >> +void >> +free_node (tree node) >> +{ >> + enum tree_code code = TREE_CODE (node); >> + if (GATHER_STATISTICS) >> + { >> + tree_code_counts[(int) TREE_CODE (node)]--; >> + tree_node_counts[(int) t_kind]--; >> + tree_node_sizes[(int) t_kind] -= tree_code_size (TREE_CODE >(node)); >> + } >> + if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) >> + vec_free (CONSTRUCTOR_ELTS (node)); >> + else if (code == BLOCK) >> + vec_free (BLOCK_NONLOCALIZED_VARS (node)); >> + else if (code == TREE_BINFO) >> + vec_free (BINFO_BASE_ACCESSES (node)); >> + ggc_free (node); >> +} >> >> /* Return a new node with the same contents as NODE except that its >> TREE_CHAIN, if it has one, is zero and it has a fresh uid. */ >> @@ -7100,12 +7123,7 @@ type_hash_canon (unsigned int hashcode, >> { >> tree t1 = ((type_hash *) *loc)->type; >> gcc_assert (TYPE_MAIN_VARIANT (t1) == t1); >> - if (GATHER_STATISTICS) >> - { >> - tree_code_counts[(int) TREE_CODE (type)]--; >> - tree_node_counts[(int) t_kind]--; >> - tree_node_sizes[(int) t_kind] -= sizeof (struct >tree_type_non_common); >> - } >> + free_node (type); >> return t1; >> } >> else >> Index: tree.h >> =================================================================== >> --- tree.h (revision 230718) >> +++ tree.h (working copy) >> @@ -3763,6 +3763,10 @@ extern int allocate_decl_uid (void); >> extern tree make_node_stat (enum tree_code MEM_STAT_DECL); >> #define make_node(t) make_node_stat (t MEM_STAT_INFO) >> >> +/* Free tree node. */ >> + >> +extern void free_node (tree); >> + >> /* Make a copy of a node, with all the same contents. */ >> >> extern tree copy_node_stat (tree MEM_STAT_DECL); >> Index: lto/lto.c >> =================================================================== >> --- lto/lto.c (revision 230718) >> +++ lto/lto.c (working copy) >> @@ -1623,13 +1622,9 @@ unify_scc (struct data_in *data_in, unsi >> data_in->location_cache.revert_location_cache (); >> for (unsigned i = 0; i < len; ++i) >> { >> - enum tree_code code; >> if (TYPE_P (scc->entries[i])) >> num_merged_types++; >> - code = TREE_CODE (scc->entries[i]); >> - if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) >> - vec_free (CONSTRUCTOR_ELTS (scc->entries[i])); >> - ggc_free (scc->entries[i]); >> + free_node (scc->entries[i]); >> } >> >> break;
Index: tree.c =================================================================== --- tree.c (revision 230718) +++ tree.c (working copy) @@ -1103,6 +1103,27 @@ make_node_stat (enum tree_code code MEM_ return t; } + +/* Free tree node. */ + +void +free_node (tree node) +{ + enum tree_code code = TREE_CODE (node); + if (GATHER_STATISTICS) + { + tree_code_counts[(int) TREE_CODE (node)]--; + tree_node_counts[(int) t_kind]--; + tree_node_sizes[(int) t_kind] -= tree_code_size (TREE_CODE (node)); + } + if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) + vec_free (CONSTRUCTOR_ELTS (node)); + else if (code == BLOCK) + vec_free (BLOCK_NONLOCALIZED_VARS (node)); + else if (code == TREE_BINFO) + vec_free (BINFO_BASE_ACCESSES (node)); + ggc_free (node); +} /* Return a new node with the same contents as NODE except that its TREE_CHAIN, if it has one, is zero and it has a fresh uid. */ @@ -7100,12 +7123,7 @@ type_hash_canon (unsigned int hashcode, { tree t1 = ((type_hash *) *loc)->type; gcc_assert (TYPE_MAIN_VARIANT (t1) == t1); - if (GATHER_STATISTICS) - { - tree_code_counts[(int) TREE_CODE (type)]--; - tree_node_counts[(int) t_kind]--; - tree_node_sizes[(int) t_kind] -= sizeof (struct tree_type_non_common); - } + free_node (type); return t1; } else Index: tree.h =================================================================== --- tree.h (revision 230718) +++ tree.h (working copy) @@ -3763,6 +3763,10 @@ extern int allocate_decl_uid (void); extern tree make_node_stat (enum tree_code MEM_STAT_DECL); #define make_node(t) make_node_stat (t MEM_STAT_INFO) +/* Free tree node. */ + +extern void free_node (tree); + /* Make a copy of a node, with all the same contents. */ extern tree copy_node_stat (tree MEM_STAT_DECL); Index: lto/lto.c =================================================================== --- lto/lto.c (revision 230718) +++ lto/lto.c (working copy) @@ -1623,13 +1622,9 @@ unify_scc (struct data_in *data_in, unsi data_in->location_cache.revert_location_cache (); for (unsigned i = 0; i < len; ++i) { - enum tree_code code; if (TYPE_P (scc->entries[i])) num_merged_types++; - code = TREE_CODE (scc->entries[i]); - if (CODE_CONTAINS_STRUCT (code, TS_CONSTRUCTOR)) - vec_free (CONSTRUCTOR_ELTS (scc->entries[i])); - ggc_free (scc->entries[i]); + free_node (scc->entries[i]); } break;