Re: RFA (GGC): PATCH to support GGC finalizers with PCH
On Tue, Nov 17, 2015 at 8:46 PM, Jason Merrill wrote: > On 11/17/2015 09:39 AM, Richard Biener wrote: >> >> On Tue, Nov 17, 2015 at 3:09 PM, Jason Merrill wrote: >>> >>> While I was looking at the interaction of delayed folding with GGC, I >>> noticed that ggc_handle_finalizers currently runs no finalizers if >>> G.context_depth != 0. So any GC objects in a greater depth will still be >>> collected, but they won't have their finalizers run. This specifically >>> affects compiles that use a PCH file, since G.context_depth is set to 1 >>> after loading the PCH. >>> >>> This patch fixes ggc_handle_finalizers to look at the depth of each >>> finalizer so that we still don't try to run finalizers for >>> non-collectable >>> objects loaded from the PCH, but we do run finalizers for collectable >>> objects allocated after loading the PCH. >>> >>> I ended up not relying on this for delayed folding, but it still seems >>> like >>> a good bug fix. >>> >>> Tested x86_64-pc-linux-gnu. OK for trunk? >> >> >> Hmm, this enlarges finalizer/vec_finalizer. Wouldn't it be better to >> add separate finalizer vectors for context_depth != 0? (I'm proposing >> to add one for exactly context_depth == 1) >> >> When is context_depth increased other than for PCH? > > > That seems to be the only place it's changed currently. I was assuming that > the generalized way ggc-page handles context_depth was intended to support > more depths in the future (perhaps for collecting after processing a nested > function?), so my patch was following that model. > > How about this? Looks good apart from the .safe_grow_cleared () which should probably better do a .safe_push (vNULL)? ("cleared" exposes too much of an implementation detail for vec<>) Thanks, Richard. > > Jason >
Re: RFA (GGC): PATCH to support GGC finalizers with PCH
On 11/17/2015 09:39 AM, Richard Biener wrote: On Tue, Nov 17, 2015 at 3:09 PM, Jason Merrill wrote: While I was looking at the interaction of delayed folding with GGC, I noticed that ggc_handle_finalizers currently runs no finalizers if G.context_depth != 0. So any GC objects in a greater depth will still be collected, but they won't have their finalizers run. This specifically affects compiles that use a PCH file, since G.context_depth is set to 1 after loading the PCH. This patch fixes ggc_handle_finalizers to look at the depth of each finalizer so that we still don't try to run finalizers for non-collectable objects loaded from the PCH, but we do run finalizers for collectable objects allocated after loading the PCH. I ended up not relying on this for delayed folding, but it still seems like a good bug fix. Tested x86_64-pc-linux-gnu. OK for trunk? Hmm, this enlarges finalizer/vec_finalizer. Wouldn't it be better to add separate finalizer vectors for context_depth != 0? (I'm proposing to add one for exactly context_depth == 1) When is context_depth increased other than for PCH? That seems to be the only place it's changed currently. I was assuming that the generalized way ggc-page handles context_depth was intended to support more depths in the future (perhaps for collecting after processing a nested function?), so my patch was following that model. How about this? Jason commit afb196cd7fc176736f9ff2abf92690a7c4ae4f94 Author: Jason Merrill Date: Fri Nov 13 09:39:15 2015 -0500 Support GGC finalizers with PCH. * ggc-page.c (ggc_globals): Change finalizers and vec_finalizers to be vecs of vecs. (add_finalizer): Split out from ggc_internal_alloc. (ggc_handle_finalizers): Run finalizers for the current depth. (init_ggc, ggc_pch_read): Reserve space for finalizers. diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c index deb21bb..c3af3c8 100644 --- a/gcc/ggc-page.c +++ b/gcc/ggc-page.c @@ -361,7 +361,7 @@ private: void (*m_function)(void *); size_t m_object_size; size_t m_n_objects; - }; +}; #ifdef ENABLE_GC_ALWAYS_COLLECT /* List of free objects to be verified as actually free on the @@ -456,11 +456,11 @@ static struct ggc_globals better runtime data access pattern. */ unsigned long **save_in_use; - /* Finalizers for single objects. */ - vec finalizers; + /* Finalizers for single objects. The first index is collection_depth. */ + vec > finalizers; /* Finalizers for vectors of objects. */ - vec vec_finalizers; + vec > vec_finalizers; #ifdef ENABLE_GC_ALWAYS_COLLECT /* List of free objects to be verified as actually free on the @@ -1240,6 +1240,23 @@ ggc_round_alloc_size (size_t requested_size) return size; } +static void +add_finalizer (void *result, void (*f)(void *), size_t s, size_t n) +{ + if (f == NULL) +; + else if (n == 1) +{ + finalizer fin (result, f); + G.finalizers[G.context_depth].safe_push (fin); +} + else +{ + vec_finalizer fin (reinterpret_cast (result), f, s, n); + G.vec_finalizers[G.context_depth].safe_push (fin); +} +} + /* Allocate a chunk of memory of SIZE bytes. Its contents are undefined. */ void * @@ -1387,11 +1404,8 @@ ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t n /* For timevar statistics. */ timevar_ggc_mem_total += object_size; - if (f && n == 1) -G.finalizers.safe_push (finalizer (result, f)); - else if (f) -G.vec_finalizers.safe_push - (vec_finalizer (reinterpret_cast (result), f, s, n)); + if (f) +add_finalizer (result, f, s, n); if (GATHER_STATISTICS) { @@ -1788,6 +1802,9 @@ init_ggc (void) G.by_depth_max = INITIAL_PTE_COUNT; G.by_depth = XNEWVEC (page_entry *, G.by_depth_max); G.save_in_use = XNEWVEC (unsigned long *, G.by_depth_max); + + G.finalizers.safe_grow_cleared (1); + G.vec_finalizers.safe_grow_cleared (1); } /* Merge the SAVE_IN_USE_P and IN_USE_P arrays in P so that IN_USE_P @@ -1875,36 +1892,42 @@ clear_marks (void) static void ggc_handle_finalizers () { - if (G.context_depth != 0) -return; - - unsigned length = G.finalizers.length (); - for (unsigned int i = 0; i < length;) + unsigned dlen = G.finalizers.length(); + for (unsigned d = G.context_depth; d < dlen; ++d) { - finalizer &f = G.finalizers[i]; - if (!ggc_marked_p (f.addr ())) + vec &v = G.finalizers[d]; + unsigned length = v.length (); + for (unsigned int i = 0; i < length;) { - f.call (); - G.finalizers.unordered_remove (i); - length--; + finalizer &f = v[i]; + if (!ggc_marked_p (f.addr ())) + { + f.call (); + v.unordered_remove (i); + length--; + } + else + i++; } - else - i++; } - - length = G.vec_finalizers.length (); - for (unsigned int i = 0; i < length;) + gcc_assert (dlen == G.vec_finalizers.length()); + for (unsigned d = G.context_depth; d < dlen; ++d) { -
Re: RFA (GGC): PATCH to support GGC finalizers with PCH
On Tue, Nov 17, 2015 at 3:09 PM, Jason Merrill wrote: > While I was looking at the interaction of delayed folding with GGC, I > noticed that ggc_handle_finalizers currently runs no finalizers if > G.context_depth != 0. So any GC objects in a greater depth will still be > collected, but they won't have their finalizers run. This specifically > affects compiles that use a PCH file, since G.context_depth is set to 1 > after loading the PCH. > > This patch fixes ggc_handle_finalizers to look at the depth of each > finalizer so that we still don't try to run finalizers for non-collectable > objects loaded from the PCH, but we do run finalizers for collectable > objects allocated after loading the PCH. > > I ended up not relying on this for delayed folding, but it still seems like > a good bug fix. > > Tested x86_64-pc-linux-gnu. OK for trunk? Hmm, this enlarges finalizer/vec_finalizer. Wouldn't it be better to add separate finalizer vectors for context_depth != 0? (I'm proposing to add one for exactly context_depth == 1) When is context_depth increased other than for PCH? Richard.
RFA (GGC): PATCH to support GGC finalizers with PCH
While I was looking at the interaction of delayed folding with GGC, I noticed that ggc_handle_finalizers currently runs no finalizers if G.context_depth != 0. So any GC objects in a greater depth will still be collected, but they won't have their finalizers run. This specifically affects compiles that use a PCH file, since G.context_depth is set to 1 after loading the PCH. This patch fixes ggc_handle_finalizers to look at the depth of each finalizer so that we still don't try to run finalizers for non-collectable objects loaded from the PCH, but we do run finalizers for collectable objects allocated after loading the PCH. I ended up not relying on this for delayed folding, but it still seems like a good bug fix. Tested x86_64-pc-linux-gnu. OK for trunk? commit 0bd746ae39b37b9b08e4d861d97fe30ecf4e8ad8 Author: Jason Merrill Date: Fri Nov 13 09:39:15 2015 -0500 Support GGC finalizers with PCH. * ggc-page.c (class finalizer): Add m_depth field. (finalizer::finalizer): Initialize it. (finalizer::depth): Return it. (class vec_finalizer): Likewise. (ggc_internal_alloc): Adjust constructor calls. (ggc_handle_finalizers): Run finalizers that are deep enough. diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c index deb21bb..1d5aeef 100644 --- a/gcc/ggc-page.c +++ b/gcc/ggc-page.c @@ -331,22 +331,26 @@ typedef struct page_table_chain class finalizer { public: - finalizer (void *addr, void (*f)(void *)) : m_addr (addr), m_function (f) {} + finalizer (void *addr, void (*f)(void *), unsigned short depth) +: m_addr (addr), m_function (f), m_depth(depth) {} void *addr () const { return m_addr; } - + unsigned short depth () const { return m_depth; } void call () const { m_function (m_addr); } private: void *m_addr; void (*m_function)(void *); + unsigned short m_depth; }; class vec_finalizer { public: - vec_finalizer (uintptr_t addr, void (*f)(void *), size_t s, size_t n) : -m_addr (addr), m_function (f), m_object_size (s), m_n_objects (n) {} + vec_finalizer (uintptr_t addr, void (*f)(void *), size_t s, size_t n, + unsigned short depth) +: m_addr (addr), m_function (f), m_object_size (s), m_n_objects (n), +m_depth (depth) {} void call () const { @@ -355,13 +359,15 @@ public: } void *addr () const { return reinterpret_cast (m_addr); } + unsigned short depth () const { return m_depth; } private: uintptr_t m_addr; void (*m_function)(void *); size_t m_object_size; size_t m_n_objects; - }; + unsigned short m_depth; +}; #ifdef ENABLE_GC_ALWAYS_COLLECT /* List of free objects to be verified as actually free on the @@ -1388,10 +1394,11 @@ ggc_internal_alloc (size_t size, void (*f)(void *), size_t s, size_t n timevar_ggc_mem_total += object_size; if (f && n == 1) -G.finalizers.safe_push (finalizer (result, f)); +G.finalizers.safe_push (finalizer (result, f, G.context_depth)); else if (f) G.vec_finalizers.safe_push - (vec_finalizer (reinterpret_cast (result), f, s, n)); + (vec_finalizer (reinterpret_cast (result), f, s, n, + G.context_depth)); if (GATHER_STATISTICS) { @@ -1875,14 +1882,12 @@ clear_marks (void) static void ggc_handle_finalizers () { - if (G.context_depth != 0) -return; - unsigned length = G.finalizers.length (); for (unsigned int i = 0; i < length;) { finalizer &f = G.finalizers[i]; - if (!ggc_marked_p (f.addr ())) + if (f.depth() >= G.context_depth + && !ggc_marked_p (f.addr ())) { f.call (); G.finalizers.unordered_remove (i); @@ -1897,7 +1902,8 @@ ggc_handle_finalizers () for (unsigned int i = 0; i < length;) { vec_finalizer &f = G.vec_finalizers[i]; - if (!ggc_marked_p (f.addr ())) + if (f.depth() >= G.context_depth + && !ggc_marked_p (f.addr ())) { f.call (); G.vec_finalizers.unordered_remove (i);