On 06/16/2015 02:42 AM, Richard Sandiford wrote:
At the moment we need separate traits classes for hash_table, hash_set
and hash_map. I think that's a sign that we don't have the right
abstraction.
You're probably right.
The aim of this series is to unify the traits for hash_table and hash_set.
If that's OK, I'll look at adding default hash_map traits for the common
case in which the deleted/empty information is stored in the key rather
than the value and where hash_table traits could be used.
There were various things I needed to change along the way:
- Our one generic hasher, pointer_hash, inherits from typed_noop_remove.
In other words, it's specifically for cases where the pointer isn't
freed when an entry is deleted. That seems a bit arbitrary. I think
we should separate out the hashing and the removing aspects.
Seems wise. Certainly doesn't feel right that hashing and removing are
tied together like this (/me scurries off to see if this was my doing...)
Also, typed_noop_remove and typed_free_remove don't provide hashing typedefs,
but the ggc equivalents ggc_hasher and ggc_cache_hasher do. If we separate
the hashing and removing aspects, the typedefs belong to the hashing side.
Right.
Much of the awkwardness here is due to not having "proper" object
construction and destruction. If we had that, the memory management
would be an integral part of the element type rather than being part
of the traits. So I don't think adding a template parameter for the
element remover would necessarily be forward progress.
It's a common theme in GCC.
- handle_cache_entry is too tied to the internals. The generic
implementation is:
static void
handle_cache_entry (T &e)
{
if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && !ggc_marked_p
(e))
e = static_cast<T> (HTAB_DELETED_ENTRY);
}
and a typical override looks like:
void
pad_type_hasher::handle_cache_entry (pad_type_hash *&t)
{
extern void gt_ggc_mx (pad_type_hash *&);
if (t == HTAB_EMPTY_ENTRY || t == HTAB_DELETED_ENTRY)
return;
else if (ggc_marked_p (t->type))
gt_ggc_mx (t);
else
t = static_cast<pad_type_hash *> (HTAB_DELETED_ENTRY);
}
In particular, this bypasses the normal remove() function, so we never
update m_n_deleted.
ISTM the (single) caller should be checking for empty and deleted items
first and handling any deletion itself. All we want to know from the
traits is which of the following applies:
- the entry should be deleted
- the entry should be kept and needs to be marked
- the entry should be kept and is already marked (an optimisation of
the previous case).
The patch adds a keep_cache_entry function to provide this information.
Seems like this is a result of having the memory management being part
of the traits rather than the element.
Thanks for the detailed overview... Onward to the patches.
jeff
