On Tue, Jun 16, 2015 at 09:45:56AM +0100, Richard Sandiford wrote:
> As described in the covering note, this patch replaces handle_cache_entry
> with a new function keep_cache_entry. It also ensures that elements are
> deleted using the proper function, so that m_n_deleted is updated.
Thanks for taking this off my to do list ;-)
> I couldn't tell whether the unusual name of the function
> ("gt_cleare_cache") is deliberate or not, but I left it be.
I'm not sure what's particularly unusual about it?
> + static int
> + keep_cache_entry (tree_int_map *&m)
I think we could now change the interface to take const T *? I imagine
inlining may get rid of the extra indirection anyway, but it feels
cleaner anyway.
> + - An optional static function named 'keep_cache_entry'. This
> + function is provided only for garbage-collected elements that
> + are not marked by the normal gc mark pass. It describes what
> + what should happen to the element at the end of the gc mark phase.
> + The return value should be:
> + - 0 if the element should be deleted
> + - 1 if the element should be kept and needs to be marked
> + - -1 if the element should be kept and is already marked.
> + Returning -1 rather than 1 is purely an optimization.
In theory using an enum seems better, but I'm not sure if the extra
verbosity makes it better in practice.
> + static int
> + keep_cache_entry (T &e)
> {
> - if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && !ggc_marked_p
> (e))
> - e = static_cast<T> (HTAB_DELETED_ENTRY);
> + return ggc_marked_p (e) ? -1 : 0;
hmm, this is the only place where -1 is used right? I believe this
case only works if the hash table is storing pointers to things in gc
memory, and only keeps things that get marked by other paths. I
believe that means -1 is only a very small optimization because in the
case we return -1 all we save is the check that that pointer itself is
marked. So i'm tempted to change this interface to just return a bool.
Of course it would be nice if the compiler could inline enough to
actually optimize out the redundant check if the pointer is makred, but
that might take some shuffling code around.
thanks for cleaning this up, and sorry my response is delayed.
Trev
