Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating std::hast<EntityId_t> to get a better unordered containers distribution [6895] #868

Merged
merged 2 commits into from
Feb 11, 2020

Conversation

MiguelBarro
Copy link
Contributor

Internally STL implementation of hash tables (unordered_map and set) use modulus operation to choose in which bucket put each element. Using an uniform distributed hash guarantees that the buckets get evenly filled and access time is minimum.
Until now we were using the whole EntityId_t interpret as a number as hash. That led to a non-uniform, unsuitable distribution because within each EntityId_t are kept flags not merely a counter. Now, only the counter value is used solving the speed issue.

Copy link
Contributor

@IkerLuengo IkerLuengo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this still lead to an uneven distribution? The hash output is UINT32, but we are using only the 3 least significant octets, leaving a very small subset of values on use

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status
  • Performance Build Status

Copy link
Member

@MiguelCompany MiguelCompany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor style comments only

include/fastrtps/rtps/common/EntityId_t.hpp Outdated Show resolved Hide resolved
include/fastrtps/rtps/common/EntityId_t.hpp Show resolved Hide resolved
@MiguelCompany
Copy link
Member

Wouldn't this still lead to an uneven distribution? The hash output is UINT32, but we are using only the 3 least significant octets, leaving a very small subset of values on use

In fact, we are using the 3 MOST significant octets, thus removing the fourth one, which always the entity kind, and will have almost always the same value.

Right now, the only place where this hash function is used is on unordered_map structures that keep the list of readers and the list of writers of the same participant. As the participant creates entities by incrementing a counter that is assigned to the 3 first octets of the entityId, this PR ensures uniform distributions on those structures.

@MiguelBarro
Copy link
Contributor Author

Is our responsibility to select the most suitable number of buckets for the hash tables. Improvements into static allocations using foonathan allocated unordered maps have not yet been introduced in 1.9.x, thus it's not clear, but when a number of expected endpoints is provided the hash tables constructors in use rehash the table to a proper number of buckets.

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status
  • Performance Build Status

@MiguelBarro MiguelBarro changed the title Updating std::hast<EntityId_t> to get a better unordered containers distribution Updating std::hast<EntityId_t> to get a better unordered containers distribution [6895] Nov 20, 2019
@MiguelCompany MiguelCompany added this to the v1.9.5 milestone Jan 21, 2020
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany MiguelCompany merged commit 4134783 into 1.9.x Feb 11, 2020
@MiguelCompany MiguelCompany deleted the hotfix/hash_EntityId_t branch February 11, 2020 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants