remove UniqueIDHasher #1957
remove UniqueIDHasher #1957
Conversation
|
Test PASSed. |
|
Thanks for the PR. Have you had a chance to evaluate whether this change has any performance impact? |
src/ray/id.h
Outdated
| @@ -96,6 +88,22 @@ const TaskID ComputeTaskId(const ObjectID &object_id); | |||
| /// if created by a put. | |||
| int64_t ComputeObjectIndex(const ObjectID &object_id); | |||
|
|
|||
| size_t hash_UniqueID(const ::ray::UniqueID& id); | |||
There was a problem hiding this comment.
Is this used anywhere? If not, let's remove it!
There was a problem hiding this comment.
It is a mistake, I will remove it.
|
Thanks for the PR! |
|
Can you fix the linting errors (shown in https://travis-ci.org/ray-project/ray/jobs/371987419). The other test failures look unrelated to me. |
| @@ -81,6 +81,12 @@ bool UniqueID::operator==(const UniqueID &rhs) const { | |||
| return std::memcmp(data(), rhs.data(), kUniqueIDSize) == 0; | |||
| } | |||
|
|
|||
| size_t UniqueID::hash() const { | |||
| size_t result; | |||
| std::memcpy(&result, id_, sizeof(size_t)); | |||
There was a problem hiding this comment.
@stephanie-wang this hashing scheme will likely be a problem with the new way xray is encoding information in the Object IDs, right?
I don't want to address it in this PR, since this PR should not change the hash function, but just want to bring it up.
There was a problem hiding this comment.
I reused the old hash function in the PR. We can change the hash function when we change the Object IDs' encoding.
And we can provide different hash function for different ID. We also changed the hash function for the plasma ObjectID in our internal branch, it will not affect it.
There was a problem hiding this comment.
Do you have any idea how to repro the https://travis-ci.org/ray-project/ray/jobs/371987419, I have run the "python test/runtest.py" already, but no error raised.
There was a problem hiding this comment.
You can either copy the diff from travis or run some variant of this:
~/ray/.travis/git-clang-format --binary clang-format --commit 3c76461b225eb91a86cc0802e20a04e65b5b408a --diff --exclude '(.*thirdparty/|.*redismodule.h|.*webui*)'
where 3c76461 should be the parent commit of your PR.
|
Test PASSed. |
|
Test PASSed. |
|
@eric-jj There is still a small linting error remaining -- can you also fix that? |
|
Test PASSed. |
|
Test FAILed. |
|
jenkins retest this please |
|
Test PASSed. |
* 'master' of https://github.com/ray-project/ray: [rllib] Fix broken link in docs (ray-project#1967) [DataFrame] Sample implement (ray-project#1954) [DataFrame] Implement Inter-DataFrame operations (ray-project#1937) remove UniqueIDHasher (ray-project#1957) [rllib] Add DDPG documentation, rename DDPG2 <=> DDPG (ray-project#1946) updates (ray-project#1958) Pin Cython in autoscaler development example. (ray-project#1951) Incorporate C++ Buffer management and Seal global threadpool fix from arrow (ray-project#1950) [XRay] Add consistency check for protocol between node_manager and local_scheduler_client (ray-project#1944) Remove smart_open install. (ray-project#1943) [DataFrame] Fully implement append, concat and join (ray-project#1932) [DataFrame] Fix for __getitem__ string indexing (ray-project#1939) [DataFrame] Implementing write methods (ray-project#1918) [rllib] arr[end] was excluded when end is not None (ray-project#1931) [DataFrame] Implementing API correct groupby with aggregation methods (ray-project#1914)
This is jin jiang from ant finance.
I will try to integrate the changes we did in the ant finance for ray to master branch.
For the change, it will remove the UniqueIDHasher struct and use the a struct in std namespace to replace it, which will reduce the complex for the stl container declaration like map, set, unordered_map.