[BBT#572] Implement hash table API providing a key handle during lock

[devreal]

Original PR: https://bitbucket.org/icldistcomp/parsec/pull-requests/572

In TTG we use the following pattern to find/insert/remove items in a hash table bucket in the critical path of sending data along edges: (similar code can be found in PaRSEC/DPLASMA)

parsec_hash_table_lock_bucket(ht, key);
item = parsec_hash_table_nolock_find(ht, key);
if (NULL == item) {
  item = create_new(key);
  parsec_hash_table_nolock_insert(ht, item);
} else if (is_complete(item)) {
  parsec_hash_table_nolock_remove(ht, key);
}
parsec_hash_table_unlock_bucket(ht, key);

This code computes the hash of key at least 3 times, potentially a fourth time if the item is inserted or removed. Hashing is rather expensive and involves a) an indirect call to a user-provided function where we don’t know how expensive it is; and b) compression of the hash value to the number of bits currently used (not terribly expensive, but probably a few dozen instructions each time).

This PR adds a key handle that caches the hash information and that is provided by parsec_hash_table_lock_bucket_handle. The handle can be used to insert, remove, and find items as well as when unlocking the bucket. The above code then becomes:

parsec_key_handle_t kh;
parsec_hash_table_lock_bucket_handle(ht, key, &kh);
item = parsec_hash_table_nolock_find_handle(ht, key, &kh);
if (NULL == item) {
  item = create_new(key);
  parsec_hash_table_nolock_insert_handle(ht, &kh, item);
} else if (is_complete(item)) {
  parsec_hash_table_nolock_remove_handle(ht, &kh);
}
parsec_hash_table_unlock_bucket_handle(ht, &kh);

In this case, the key is hashed only once throughout this sequence in the call to lock the bucket. The handle is valid until the bucket is unlocked since no resizing may occur while a bucket is locked. The insert function ignores the key of the item and uses the key stored in the handle instead. It is unsafe to use a key in item that is different from the key used to lock the bucket, so IMO that is safe.

The handle stores the key as well as the compressed and uncompressed hash values:

struct parsec_key_handle_s {
    uint64_t                  hash64;           /**< Is a 64-bits hash of the key */
    uint64_t                  hash;             /**< Is a 64-bits hash of the key,
                                                     trimmed to the current size of the table */
    parsec_key_t              key;              /**< Items are identified with this key */
};

The impact is measurable, albeit not overwhelming (about 10% in the TTG benchmark stressing the data flow handling). The changes are not terribly invasive and recomputing the hash every time is somewhat of a waste, no matter what.

I converted the code in datarepo.c to show how it looks like in code. If approved, I will convert more code that uses similar patterns.

Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu

[bosilca]

Please rebase this PR on current master to get the CI going.

[devreal]

Should I port all usages of parsec_hash_table_lock_bucket to parsec_hash_table_lock_bucket_handle?

[devreal]

I ported almost all use-cases to the new handle API. There are some exceptions where the lock is taken and then nolock operations are done in a different function. I didn't touch these codes (DTD is among them and I'm scared to break it).

Using the tests/class/hash benchmark on my local laptop, I get 3595947171ns without handles and 3324471844ns with handles (7.5% speedup).

We talked about removing the old API but that's not as easy as I thought. The key is stored in the handle and then not passed explicitly to the find, remove, and unlock functions so we won't have a key there if the user didn't pass a handle pointer to lock. I'd say we simply keep the old API for convenience.