[dthain]

Proposed Changes

Our current hash_table has evolved over a number of years and revealed some weaknesses in semantics around the (single) iterator and the closed-addressing linked list implementation. While it can be used correctly, it is tricky to avoid the disallowed patterns (nested iteration, deletion during iteration, etc) that may cause further trouble down the road.

This is a first pass at improving the data structure, with some slight API changes. For the sake of this WIP, I am calling the new one hash_map but it could be renamed hash_table in the future. Key differences:

• Open addressing (array of entries) instead of closed addressing (array of lists of entries) simplifies a number of operations. it seems to result in slightly worse insert performance, and slightly better remove performance.
• Independent cursors and HASH_MAP_ITERATE (like list.h) operate using a simple integer position, allowing for nested iteration without confusion. The obvious thing works correctly.
• hash_map keeps an internal version number which is updated on each rehash. If a cursor notices a version change, it immediately goes to the invalid position, resulting in a truncated but still safe iteration over values.
• hash_map_insert and hash_map_remove can be safely performed while cursors are active. However, if either triggers a rehash, cursors jump to invalid, so the iteration ends safely but prematurely. This may be surprising behavior.
• hash_map_remove_stable removes an item but suppresses the rehash, guaranteeing cursor stability. This one should be used inside an iteration.

I'm sure that I haven't sorted out all the implementation bugs yet. And there are also some needed features to bring over from hash_table, like random iteration initialization. Also some documentation to be added. Would like feedback on the usability of this approach.

Merge Checklist

The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.

• make test Run local tests prior to pushing.
• make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
• make lint Run lint on source code prior to pushing.
• Manual Update: Update the manual to reflect user-visible changes.
• Type Labels: Select a github label for the type: bugfix, enhancement, etc.
• Product Labels: Select a github label for the product: TaskVine, Makeflow, etc.
• PR RTM: Mark your PR as ready to merge.

[dthain]

New hash_map performance: It's a little slower on inserts, and a little faster on removes. Maybe some room for improvement.

(cctools-dev) bash-4.4$ ./hash_map_benchmark 
INSERTION PHASE:
step   0 size        1 buckets      127 load_max 0.007874 load_now 0.007874 time 0.000001 time_norm 0.000001
step   1 size        3 buckets      127 load_max 0.023622 load_now 0.023622 time 0.000001 time_norm 0.000000
step   2 size        7 buckets      127 load_max 0.055118 load_now 0.055118 time 0.000001 time_norm 0.000000
step   3 size       15 buckets      127 load_max 0.118110 load_now 0.118110 time 0.000003 time_norm 0.000000
step   4 size       31 buckets      127 load_max 0.244094 load_now 0.244094 time 0.000007 time_norm 0.000000
step   5 size       63 buckets      255 load_max 0.314961 load_now 0.247059 time 0.000022 time_norm 0.000001
step   6 size      127 buckets      511 load_max 0.313725 load_now 0.248532 time 0.000088 time_norm 0.000001
step   7 size      255 buckets     1023 load_max 0.311155 load_now 0.249267 time 0.000306 time_norm 0.000002
step   8 size      511 buckets     2047 load_max 0.310850 load_now 0.249634 time 0.001377 time_norm 0.000005
step   9 size     1023 buckets     4095 load_max 0.310210 load_now 0.249817 time 0.006263 time_norm 0.000012
step  10 size     2047 buckets     8191 load_max 0.310134 load_now 0.249908 time 0.027871 time_norm 0.000027
step  11 size     4095 buckets    16383 load_max 0.310096 load_now 0.249954 time 0.113732 time_norm 0.000056
step  12 size     8191 buckets    32767 load_max 0.310016 load_now 0.249977 time 0.447491 time_norm 0.000109
step  13 size    16383 buckets    65535 load_max 0.310007 load_now 0.249989 time 1.827925 time_norm 0.000223
step  14 size    32767 buckets   131071 load_max 0.310002 load_now 0.249994 time 7.436308 time_norm 0.000454
step  15 size    65535 buckets   262143 load_max 0.310008 load_now 0.249997 time 42.776742 time_norm 0.001305
REMOVAL PHASE:
step  15 size    32767 buckets   262143 load_min 0.124997 load_now 0.124997 time 44.340761 time_norm 0.001353
step  14 size    16383 buckets   131071 load_min 0.100003 load_now 0.124993 time 9.106636 time_norm 0.000556
step  13 size     8191 buckets    65535 load_min 0.100007 load_now 0.124987 time 2.101467 time_norm 0.000257
step  12 size     4095 buckets    32767 load_min 0.100008 load_now 0.124973 time 0.523875 time_norm 0.000128
step  11 size     2047 buckets    16383 load_min 0.100009 load_now 0.124947 time 0.131780 time_norm 0.000064
step  10 size     1023 buckets     8191 load_min 0.100043 load_now 0.124893 time 0.032536 time_norm 0.000032
step   9 size      511 buckets     4095 load_min 0.100110 load_now 0.124786 time 0.007770 time_norm 0.000015
step   8 size      255 buckets     2047 load_min 0.100122 load_now 0.124573 time 0.001793 time_norm 0.000007
step   7 size      127 buckets     1023 load_min 0.100147 load_now 0.124145 time 0.000424 time_norm 0.000003
step   6 size       63 buckets      511 load_min 0.100684 load_now 0.123288 time 0.000104 time_norm 0.000002
step   5 size       31 buckets      255 load_min 0.101761 load_now 0.121569 time 0.000028 time_norm 0.000001
step   4 size       15 buckets      127 load_min 0.101961 load_now 0.118110 time 0.000008 time_norm 0.000000
step   3 size        7 buckets      127 load_min 0.055118 load_now 0.055118 time 0.000003 time_norm 0.000000
step   2 size        3 buckets      127 load_min 0.023622 load_now 0.023622 time 0.000001 time_norm 0.000000
step   1 size        1 buckets      127 load_min 0.007874 load_now 0.007874 time 0.000001 time_norm 0.000000

Existing hash_table performance:

(cctools-dev) bash-4.4$ ./hash_table_benchmark 
INSERTION PHASE:
step   0 size        1 buckets      127 load_max 0.007874 load_now 0.007874 time 0.000000 time_norm 0.000000
step   1 size        3 buckets      127 load_max 0.023622 load_now 0.023622 time 0.000000 time_norm 0.000000
step   2 size        7 buckets      127 load_max 0.055118 load_now 0.055118 time 0.000001 time_norm 0.000000
step   3 size       15 buckets      127 load_max 0.118110 load_now 0.118110 time 0.000002 time_norm 0.000000
step   4 size       31 buckets      127 load_max 0.244094 load_now 0.244094 time 0.000005 time_norm 0.000000
step   5 size       63 buckets      127 load_max 0.496063 load_now 0.496063 time 0.000013 time_norm 0.000000
step   6 size      127 buckets      255 load_max 0.755906 load_now 0.498039 time 0.000046 time_norm 0.000001
step   7 size      255 buckets      511 load_max 0.752941 load_now 0.499022 time 0.000173 time_norm 0.000001
step   8 size      511 buckets     1023 load_max 0.751468 load_now 0.499511 time 0.000718 time_norm 0.000003
step   9 size     1023 buckets     2047 load_max 0.750733 load_now 0.499756 time 0.003420 time_norm 0.000007
step  10 size     2047 buckets     4095 load_max 0.750366 load_now 0.499878 time 0.018442 time_norm 0.000018
step  11 size     4095 buckets     8191 load_max 0.750183 load_now 0.499939 time 0.082478 time_norm 0.000040
step  12 size     8191 buckets    16383 load_max 0.750092 load_now 0.499969 time 0.351547 time_norm 0.000086
step  13 size    16383 buckets    32767 load_max 0.750046 load_now 0.499985 time 1.470138 time_norm 0.000179
step  14 size    32767 buckets    65535 load_max 0.750023 load_now 0.499992 time 6.036509 time_norm 0.000368
step  15 size    65535 buckets   131071 load_max 0.750011 load_now 0.499996 time 31.132525 time_norm 0.000950
REMOVAL PHASE:
step  15 size    32767 buckets   131071 load_min 0.249994 load_now 0.249994 time 59.326570 time_norm 0.001811
step  14 size    16383 buckets    65535 load_min 0.124993 load_now 0.249989 time 12.820147 time_norm 0.000782
step  13 size     8191 buckets    32767 load_min 0.124987 load_now 0.249977 time 3.084875 time_norm 0.000377
step  12 size     4095 buckets    16383 load_min 0.124973 load_now 0.249954 time 0.757479 time_norm 0.000185
step  11 size     2047 buckets     8191 load_min 0.124947 load_now 0.249908 time 0.185264 time_norm 0.000090
step  10 size     1023 buckets     4095 load_min 0.124893 load_now 0.249817 time 0.045349 time_norm 0.000044
step   9 size      511 buckets     2047 load_min 0.124786 load_now 0.249634 time 0.010763 time_norm 0.000021
step   8 size      255 buckets     1023 load_min 0.124573 load_now 0.249267 time 0.002375 time_norm 0.000009
step   7 size      127 buckets      511 load_min 0.124145 load_now 0.248532 time 0.000538 time_norm 0.000004
step   6 size       63 buckets      255 load_min 0.123288 load_now 0.247059 time 0.000148 time_norm 0.000002
step   5 size       31 buckets      127 load_min 0.121569 load_now 0.244094 time 0.000052 time_norm 0.000002
step   4 size       15 buckets      127 load_min 0.118110 load_now 0.118110 time 0.000018 time_norm 0.000001
step   3 size        7 buckets      127 load_min 0.055118 load_now 0.055118 time 0.000004 time_norm 0.000001
step   2 size        3 buckets      127 load_min 0.023622 load_now 0.023622 time 0.000002 time_norm 0.000000
step   1 size        1 buckets      127 load_min 0.007874 load_now 0.007874 time 0.000001 time_norm 0.000000

[btovar]

hash_map keeps an internal version number which is updated on each rehash. If a cursor notices a version change, it immediately goes to the invalid position, resulting in a truncated but still safe iteration over values.

I feel that this one is going to hide bugs and generate surprise behavior that is hard to reproduce:

1. Skipping entries may declare a task unable to run and returned to the user as failure. (E.g., we could not find the input file?).
2. For iterations that collect and delete it may create a double free?

It may be memory safe, but not application safe?

Instead what if we keep a count of iterators active. If an iterator is active then no new inserts are allowed. Removals are ok, but rehashes only occur when there are no iterators active, or if the iterator count goes to zero.

[dthain]

I think that's a reasonable approach. But as far as I can tell, every option has its poison:

• Option 0 (current hash_table) insert can result in resize, upsetting iteration order.
• Option 1: insert/remove can result in resize, forcing invalid cursors and truncated iteration.
• Option 2: insert prohibited during iteration, remove without rehash allowed. Assume that an attempted insert results in a fatal() so it can be found? This is easy to implement -- does it prohibit some important use case?
• Option 3: insert/remove allowed during iteration, but rehash is inhibited. This also works but has a different failure case -- if you have a large number of inserts with an active cursor, eventually you run out of table space and must fail somehow.

(FWIW, C++ std::unordered_map takes option 1.)

Those are the options that I can see. Are there any others?

[dthain]

Additional orthogonal idea: introduce a HASH_MAP_ITERATE_STABLE that takes a snapshot of all the keys, then iterates by doing a lookup on each of the keys in turn. This would give you stable iteration order, while inserts and deletes could be permitted and ignored. An more expensive operation of course.

The naming is a little funny, since you want the user to pick the right one. We could call the two iteration types:

• HASH_MAP_ITERATE_FAST and HASH_MAP_ITERATE_STABLE
• Or: HASH_MAP_ITERATE_UNSTABLE and HASH_MAP_ITERATE_SLOW. :)

[btovar]

Checking unordered_map insert, it returns an iterator to the inserted element, so the responsible thing to do would be to handle the rehash in the app, which sounds leaky.

Currently we have hash_table_keys_array that allows the more stable iterate. My proposal would be:

1. Never allow inserts during an iteration that uses cursors.
2. Allow erasures but do not rehash unless it is safe to do so.
3. If you really want to insert during iteration, then use HASH_TABLE_STABLE_ITERATE that calls ht keys array and frees that array on exit.

I think this eliminates all surprising behaviors?

[btovar]

In general I would say to only allow removal of the current element, and do not allow inserts during iteration because otherwise keys would be treated differently and that sound ugly. Some keys would be considered in the same iteration that they got inserted, or some keys may be deleted after they were already considered by the iteration.

[dthain]

1. Never allow inserts during an iteration that uses cursors.

Agreed. Will fatal on insert, and then we have to see if this breaks any existing code...

2. Allow erasures but do not rehash unless it is safe to do so.

Agreed.

3. If you really want to insert during iteration, then use HASH_TABLE_STABLE_ITERATE that calls ht keys array and frees that array on exit.

Agreed.

4. In general I would say to only allow removal of the current element

I think delete of any item is easy to make safe under open addressing.
Do you have a scenario in mind to limit to the cursor location?

[btovar]

Contrived but this is the idea:

count_good = 0;
iterate(h,k) {
  
    if yes(k, "bad")
        turtles(k, "bad");   //<-- deletes k=="good";

   if yes(k, "good") count++;

}
// count_good here may be 1 depending of hashing,
// but it should be always zero.

If limited to the current position of the cursor an error in counting at least it would be my fault.

[dthain]

That's a good example. I think our challenges may lie more in our patterns of usage than the data structure itself. Or rather, we need to set up good constraints to flush out the bad habits that we have now. (Or may have in the future.)

[dthain]

Ok, that is good example. I think part of our challenge here is distinguishing between problems with the data structure vs problems in our usage patterns.