TcpReassembly flowKey hash5Tuple fnvHash collision fix

[yvyw]

Fix #636. Should fix the error "Error occurred - packet doesn't match either side of the connection!!", if it is caused by hash collision.

[seladb]

@yvyw thanks for working on this PR!
It contains quite a lot of changes, can you please add some description on what these changes are? Do you think it makese sense to break it down to smaller PRs?
Also - please notice the CI fails...

[yvyw]

Hi, @seladb! Thank you for PcapPlusPlus, it's very useful!

The changes are intended to replace flowKey with the real connection object in order to eliminate collisions.
It could be implemented by replacing the flowKey with an object that contains all connection information and fits the requirements for a key in an unordered_map.
Connection objects are represented by the Packet and the ConnectionData.
Objects that can be used as a key in an unordered_map should be hashable and equality comparable. Also a key object is saved in an unordered_map on addition along with a value as a std::pair.
So, the ConnectionHashable class containing only a raw pointer was added to be used as a key in unordered_map.
The IHashableConnectionInfo interface was introduced to calculate a hash and compare any objects that can provide connection information.
The IHashableConnectionInfo adapter for a Packet and the interface implementation for a ConnectionData were added.
The hash5Tuple was adapted to calculate the hash of objects that implement the IHashableConnectionInfo interface.
The fnvHash and hash5Tuple functions were updated to calculate a hash with the size of the size_t type, as required by std::hash.
flowKey replaced with ConnectionHashable.
The tests and examples were updated to work with the changes.

Backward compatibility is not preserved because the flowKey was declared as uint32_t and the TcpReassembly::getConnectionInformation() returns a raw reference to an std::map<uint32_t,ConnectionData>.
Maybe it's a good idea for modifiability to replace flowKey type with some using and raw references to internal structures with some functions returning iterators or references.

About breaking into a smaller PR's. I think I can, but it will take time. Also, PRs would look strange without knowing the final purpose. Maybe it would be better to leave it as it is or split it into commits without creating PRs. What do you think?

Regarding CI. The project passes the tests on my machine. I will fix the errors as they appear in CI.

[seladb]

@yvyw replacing uint32_t with an object as the key of the map will make the map calculations slower. It will also increase the memory consumed by the map because the key object is bigger.

Sicne it's used in TCP Reassembly it should support high throughput and scale, and I think this implementation might cause degradation of both. Please let me know what you think.

[yvyw]

@seladb, your concerns are valid. Here's what I think:

On calculations slowdown.
A hash is calculated on search, insertion (for the object being searched/inserted) and rehashing (for all objects in the map).
Therefore, it should not slow down once the connection is added to the m_ConnectionList.
At new connection creation, the hash would be calculated twice, first at the search stage and again at the insertion stage. Although it's not as efficient as a single calculation, the speed won't deteriorate significantly unless there are a large number of short connections.
And all hashes will be recalculated when rehashing occurs. Insertion in unordered_map should still have amortized complexity of O(1), but it could introduce unnecessary delays. Although it would be better to benchmark this, but we can act preemptively by adding a method to IHashableConnectionInfo that saves the hash in the object for the future use as we are currently doing with the flowKey.
There is also a difference in the hash size for 64-bit systems. Although it's better to benchmark too, I think it would not add significant slowdown since the machine natively supports 64-bit registers.
Should I add caching for the hash?

On memory consumption.
The ConnectionHashable is the same size as a pointer. Therefore, it is the same size as the flowKey on 32-bit systems and 32 bits larger on 64-bit systems. And an additional pointer to the vptr was added to the ConnectionData.
This patch also removes the duplication of ConnectionData in TcpReassemblyData, saving 104–8 bytes on x64 systems. And removes key from the m_ConnectionList, which saves an additional 4 bytes.
So net memory consumption will even decrease after the patch.
Link to play around with the structure packing: https://godbolt.org/z/MzYKbPfMs
Furthermore, memory consumption could be reduced by replacing the IPAddress structure with a std::variant<IPv4Address,IPv6Address> and replacing the timeval in the ConnectionData with an API, that converts time_point to timeval. Even further m_ConnectionInfo could be defined as a std::forward_list, saving an additional 8 bytes per connection.

[Dimi1010]

Furthermore, memory consumption could be reduced by replacing the IPAddress structure with a std::variant<IPv4Address,IPv6Address>

I have thought about doing this, unfortunately std::variant is a C++17 construct, and we currently must support C++14.
We can reimplement it or use a backport library, but I don't think it would be worth it, compared to refactoring it when we bump the minimum supported standard.

[yvyw]

We can reimplement it or use a backport library, but I don't think it would be worth it, compared to refactoring it when we bump the minimum supported standard.

I agree. I think this is premature optimization according to Knuth, and it's better to just wait for C++17. In the comment, I wanted to say that there is some more room for memory optimization if it's required. I think projects with extremely high loads will encounter problems anyway and end up tailoring their code with a profiler.

[yvyw]

I experimented with CI errors, but I can't really understand what gcov wants. It's probably constructors, destructors, or operator= for the classes, or something else is implicitly declared in random places, which gcov does not like.
This issue can be resolved by adding --merge-mode-functions separate to GCOVR_FLAGS in .github/workflows/build_and_test.yml but I am unsure if this is the correct solution.
If you know what to do with that, please advise.

[seladb]

I experimented with CI errors, but I can't really understand what gcov wants. It's probably constructors, destructors, or operator= for the classes, or something else is implicitly declared in random places, which gcov does not like. This issue can be resolved by adding --merge-mode-functions separate to GCOVR_FLAGS in .github/workflows/build_and_test.yml but I am unsure if this is the correct solution. If you know what to do with that, please advise.

Regarding gcov - Please see this comment - for now we can downgrade it to version 8.2

[seladb]

@yvyw the IHashableConnectionInfo::equals() method is called on every map search and it's slower than calculating it once as fnvHash does.

Should I add caching for the hash?

Doesn't it mean calculating fnvHash again? 🤔

Regarding the memory consumption: I looked more closely at the code, and I think I understand why the memory consumption is roughly the same 👍

[yvyw]

the IHashableConnectionInfo::equals() method is called on every map search and it's slower than calculating it once as fnvHash does.

Yes, otherwise we cannot be sure that we have found the same connection. It's a necessary evil when dealing with hash collisions. However, fitting all the data within the same CPU cache line can improve speed, but at the expense of memory.

Doesn't it mean calculating fnvHash again?

I think it depends on the realization. We can calculate the hash when the object is constructed and then return the calculated value whenever it is needed.

[Dimi1010]

the IHashableConnectionInfo::equals() method is called on every map search and it's slower than calculating it once as fnvHash does.

Yes, otherwise we cannot be sure that we have found the same connection. It's a necessary evil when dealing with hash collisions. However, fitting all the data within the same CPU cache line can improve speed, but at the expense of memory.

@seladb @yvyw Not really familiar with the tcp reassembly code, but just throwing my 2 cents here.

I see that ConnectionInfoList has been changed from std::unordered_map<uint32_t, ConnectionData> to std::list<ConnectionData>. I assume this is related to the hash collisions on the flow key, as we might have multiple connection data's generate the same hash.

Could we use std::unordered_multimap instead directly? Essentially the find procedure would be:

1. Use current key.
2. Fetch the range of connections that match it. (std::unordered_multimap::equal_range)
3. Iterate through them to get the first equals() match.

Since the flow key duplication is an edge case, I think this would result in amortized O(1) on step 3, since most ranges will have 1 element. IMO, it should also require less overall changes from the original code, but that is for you decide as I haven't really dived deep.