-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Convert plasma/plasma_store.cc to use STL #324
Conversation
Can one of the admins verify this patch? |
The PR was rebased onto master and it compiles and passes the tests locally. |
Great, thanks a lot! I'll merge it when the tests passed. |
@@ -14,6 +14,7 @@ endif(APPLE) | |||
include_directories("${PYTHON_INCLUDE_DIRS}" thirdparty) | |||
|
|||
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --std=c99 -D_XOPEN_SOURCE=500 -D_POSIX_C_SOURCE=200809L") | |||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --std=c++11 -D_XOPEN_SOURCE=500 -D_POSIX_C_SOURCE=200809L") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is actually duplicated above (line 8). We should remove line 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
struct hash<UniqueID> { | ||
size_t operator()(const UniqueID &unique_id) const { | ||
return *reinterpret_cast<const size_t *>(unique_id.id + UNIQUE_ID_SIZE - | ||
sizeof(size_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using the last size_t
bytes of the object ID as a hash of the unique ID? Why not the following?
return *reinterpret_cast<const size_t *>(unique_id.id)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intention was that std::hash<UniqueId>() shouldn't return the same value for consecutive IDs, which I believe Philipp said was occurring somewhere. But actually, if unique_id.id is treated as a little-endian integer and then incremented, getting the last bytes in this way would cause the hash of consecutive IDs to usually be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @atumanov has another hash function which we should look into. I think we should merge it with Richards hash function now which is very reasonable and then we can see if we have a better alternative later.
Nice work @rshin! |
* Change plasma_store.c to C++ (clobbering existing FlatBuffers usage). * Convert plasma_store.cc to use STL (with a caveat) * Fix CMakeLists and mutation-while-iterating problem * Remove extra extern "C" declarations * Remove redundant -std=c++11 from plasma/CMakeLists.txt
…project#324… (ray-project#33601) The failure in rllib should have been fixed by ray-project#33562 Verified with `python -m pytest rllib/core/learner/torch/tests/test_torch_learner.py::TestLearner::test_end_to_end_update`. Signed-off-by: elliottower <elliot@elliottower.com>
…project#324… (ray-project#33601) The failure in rllib should have been fixed by ray-project#33562 Verified with `python -m pytest rllib/core/learner/torch/tests/test_torch_learner.py::TestLearner::test_end_to_end_update`. Signed-off-by: Jack He <jackhe2345@gmail.com>
No description provided.