Optimize hash table insertion

[olivier-matz-6wind]

Hello,

This is a draft pull request that solves the issue #2106

The idea is to allow to insert elements in the hash table in O(1), even in case of collisions. First, the hash table is reworked to avoid browsing records to find a free one. Then, a new API is added to allow an insertion without checking for duplicates.

Feedback is of course welcome!

Note: the first patch breaks the validation test, but it is fixed by the second. From what I understand, it is expected that the browsing order is the same than the insert order, but I think this was not always guaranteed by the previous implementation.

[olivier-matz-6wind]

I noticed the test failures, I'll fix them if we move forward with this draft.

[michalvasko]

Thanks for the effort, in general it seems fine but I have made some comments. I am also not completely sure I understand what exact inefficiencies you solved so please correct me if I have it wrong.

1. Skip the check that a value is already in the hash table for all data manipulation, is completely redundant (interestingly, it seems that way and I could not think of a use-case where it could lead to problems).
2. Avoid linear search for an empty record in case the hash-based record is taken. Implemented using a "bucket" for each record that holds the information about the first/last filled record for the hash.

Would be great if something like this (if I am right) is mentioned in the hash table description. Also, from what I remember, buckets were used as an overflow space for collisions to not fill additional hash table records in some hash table collision algorithm so I am not sure the best name was chosen for these structures. Lastly, I believe the size of hash tables has increased by size * 8 B, which is not ideal but should probably be fine.

[olivier-matz-6wind]

Thanks @michalvasko for the comments.

Thanks for the effort, in general it seems fine but I have made some comments. I am also not completely sure I understand what exact inefficiencies you solved so please correct me if I have it wrong.

1. Skip the check that a value is already in the hash table for all data manipulation, is completely redundant (interestingly, it seems that way and I could not think of a use-case where it could lead to problems).

Yes, this is achieved by the new API lyht_insert_no_check().

2. Avoid linear search for an empty record in case the hash-based record is taken. Implemented using a "bucket" for each record that holds the information about the first/last filled record for the hash.

Yes.

Would be great if something like this (if I am right) is mentioned in the hash table description.

OK, I'll update the hash table description with more details.

Also, from what I remember, buckets were used as an overflow space for collisions to not fill additional hash table records in some hash table collision algorithm so I am not sure the best name was chosen for these structures.

I can rename it as "hlist" if you feel it's more relevant.

Lastly, I believe the size of hash tables has increased by size * 8 B, which is not ideal but should probably be fine.

Yes. It is +(size * 4) after the first commit and +(size * 8) after the second commit.

While there, do you have an idea why the second commit would be needed to fix the validation unit test?

[michalvasko]

I can rename it as "hlist" if you feel it's more relevant.

Sure, just to avoid people assuming they are buckets from the other algorithm.

While there, do you have an idea why the second commit would be needed to fix the validation unit test?

Sorry, I am working on something else and would prefer not to leave it in the middle of things unless something else that is critical appears. Just do what you can and incorporate the feedback I give you, I will then take a look later if you will not be able to solve some issues.

[olivier-matz-6wind]

changelog between v1 and v2:

• rename bucket as hlist
• revert to val[1] instead of val[] in record structure
• revert to packed record structure
• better document ly_ht structure (how the hash table works)
• move lyht_insert_no_check() declaration in the proper patch
• fix API documentation of lyht_insert_no_check()
• do not use __builtin_clz() which is not available under windows
• call ly_ht_insert_no_check() on resize
• use lyht_get_fixed_size(u) in patch "optimize creation of children htable"
• fix minor format and cosmetic issues

[olivier-matz-6wind]

call ly_ht_insert_no_check() on resize

Thinking a bit more about it, I will remove this change, and revert to what I had in v1.

There is certainly something I don't understand about the need of specifying a val_eq_cb when resizing. My naive idea was that we don't need to check anything at resize, since the elements were already there. But there is an explicit API to do set the callback at resize, so my idea is probably wrong...

[olivier-matz-6wind]

changelog between v2 and v3:

• call ly_ht_insert_no_check() on resize only if the resize is triggered by a ly_ht_insert_no_check(), as it was done in v1

[olivier-matz-6wind]

changelog between v3 and v4:

• fix format issues (sorry, I didn't realize I had to install uncrustify 0.77 to pass the test)

About the ABI compatibility check failure, the problem identified by abi-dumper is in lyd_node_inner, which includes a pointer to the children hash table:

Medium Severity
Base type of field children_ht has been changed from struct hash_table to struct ly_ht of different format.
This field may be incorrectly initialized or accessed by applications.

My thinking is that this field is never accessed by an application, only by the library (moreover the hash table type was not public until quite recently), so I think it should not be an issue. Please let me know if that's not the case.

[michalvasko]

Thanks, the ABI problem should be fine.

[olivier-matz-6wind]

Thank you!