Hi Yipeng, On 09/04/2018 03:51 PM, Wang, Yipeng1 wrote:
Hmm, I guess my comment is for code readability. If we don’t need the extra state that would be great.
Notice that applications only see the public, opaque state. And the private versions are scoped to the C file where they are needed.
I think "rte_hash" is defined as an internal data structure but expose the type to the public header. Would this work?
Exposing the private fields would bind the interface with the current implementation of the hash table. In the way we are proposing, one should be able to replace the underlying algorithm and not touching the header files that applications use. But, yes, your solution would enable applications to allocate iterator states as local variables as well.
I propose to malloc inside function mostly because I think it is cleaner to the user. But your argument is valid. Depending on use case I think it is OK.
I don't know how other applications will use this iterator, but we use it when our application is already overloaded. So avoiding an expensive operation like malloc() is a win.
Another comment is you put the total_entry in the state, is it for performance of the rte_hash_iterate?
We are saving one integer multiplication per call of rte_hash_iterate(). It's not a big deal, but since there's room in the state variable, we thought this would be a good idea because it grows with the size of the table. We didn't actually measure the effect of this decision.
If you use it to iterate conflict entries, especially If you reuse same "state" struct and init it again and again for different keys, would this slow down the performance for your specific use case?
Notice that the field total_entry only exists for rte_hash_iterate(). But even if total_entry were in the state of rte_hash_iterate_conflict_entries(), it would still save on the multiplication as long as rte_hash_iterate_conflict_entries() is called at least twice. Calling rte_hash_iterate_conflict_entries() once evens out, and calling rte_hash_iterate_conflict_entries() more times adds further savings. As a side note. in our application, whenever an iterator of conflicting entries is initialized, we call rte_hash_iterate_conflict_entries() at least once.
Also iterate_conflic_entry may need reader lock protection.
We are going to add the reader lock protection. Thanks. [ ]'s Michel Machado
