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.

Sign up for GitHub

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

Jump to bottom

Convert plasma/plasma_store.cc to use STL #324

Merged
merged 5 commits into from
Apr 1, 2017
Merged

Convert plasma/plasma_store.cc to use STL #324

merged 5 commits into from
Apr 1, 2017

Conversation

rshin
Copy link
Contributor

@rshin rshin commented Feb 27, 2017

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

This was referenced Mar 24, 2017
Merged
Closed
@pcmoritz pcmoritz mentioned this pull request Mar 31, 2017
Closed
@rshin
Copy link
Contributor Author

rshin commented Apr 1, 2017

The PR was rebased onto master and it compiles and passes the tests locally.

@rshin rshin changed the title Convert Plasma to C++ Convert plasma/plasma_store.cc to use STL Apr 1, 2017
@pcmoritz
Copy link
Contributor

pcmoritz commented Apr 1, 2017

Great, thanks a lot! I'll merge it when the tests passed.

robertnishihara
robertnishihara reviewed Apr 1, 2017
View reviewed changes
src/plasma/CMakeLists.txt
@@ -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")
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

robertnishihara
robertnishihara reviewed Apr 1, 2017
View reviewed changes
src/plasma/plasma_store.cc
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));
Copy link
Collaborator

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@pcmoritz pcmoritz merged commit 227c916 into master Apr 1, 2017
@pcmoritz pcmoritz deleted the plasma-cpp branch April 1, 2017 05:58
@robertnishihara
Copy link
Collaborator

Nice work @rshin!

atumanov pushed a commit to atumanov/ray that referenced this pull request Apr 4, 2017
@rshin @atumanov
Convert plasma/plasma_store.cc to use STL (ray-project#324)
229e22b 
* 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
@rshin rshin mentioned this pull request Apr 18, 2017
Closed
ericl pushed a commit that referenced this pull request Mar 23, 2023
@jianoaix
Revert "[Datasets] Revert "Enable streaming executor by default (#324… (
c38a3cf 
#33601)

The failure in rllib should have been fixed by #33562

Verified with `python -m pytest rllib/core/learner/torch/tests/test_torch_learner.py::TestLearner::test_end_to_end_update`.
scottsun94 pushed a commit to scottsun94/ray that referenced this pull request Mar 28, 2023
@jianoaix @scottsun94
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
9f28f88 
…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`.
cassidylaidlaw pushed a commit to cassidylaidlaw/ray that referenced this pull request Mar 28, 2023
@jianoaix @cassidylaidlaw
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
b5c84d2 
…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`.
brycehuang30 pushed a commit to brycehuang30/ray that referenced this pull request Mar 29, 2023
@jianoaix @brycehuang30
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
6c2093e 
…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: bhuang <[email protected]>
joncarter1 pushed a commit to joncarter1/ray that referenced this pull request Apr 2, 2023
@jianoaix @joncarter1
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
fb43886 
…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: Jonathan Carter <[email protected]>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@jianoaix @elliottower
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
0baff8a 
…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 <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@jianoaix @ProjectsByJackHe
Revert "[Datasets] Revert "Enable streaming executor by default (ray-…
4f355bc 
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcmoritz pcmoritz pcmoritz left review comments

@robertnishihara robertnishihara robertnishihara left review comments

Assignees

@pcmoritz pcmoritz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rshin @AmplabJenkins @pcmoritz @robertnishihara