Details

    • Improvement
    • Status: Resolved
    • Minor
    • Resolution: Won't Fix
    • None
    • None
    • libprocess

    Description

      Deprecate process::Shared in favour of std::shared_ptr or std::tr1::shared_ptr from stout/memory.hpp.

      Attachments

        Issue Links

          Activity

            dhamon Dominic Hamon added a comment -

            Retain process::Shared and process::Owned but use the std:: versions within libprocess and stout.

            dhamon Dominic Hamon added a comment - Retain process::Shared and process::Owned but use the std:: versions within libprocess and stout.
            jieyu Jie Yu added a comment -

            Dominic, I think most of our shared_ptr usage in Mesos code base can be converted to unique_ptr. Yeah, we have lot of use of shared_ptr in libprocess. But that's for expert only

            I agree with you that unless we have a very strong reason to use shared_ptr, we should use process::Shared and Owned (unique_ptr) instead.

            [tw-mbp-jyu mesos (master)]$ grep -r shared_ptr src/
            src//linux/routing/internal.hpp:  memory::shared_ptr<Data> data;
            src//master/http.cpp:    foreach (const memory::shared_ptr<Task>& task, framework.completedTasks) {
            src//master/http.cpp:    foreach (const memory::shared_ptr<Framework>& framework,
            src//master/http.cpp:  foreach (const memory::shared_ptr<Framework>& framework,
            src//master/http.cpp:    foreach (const memory::shared_ptr<Task>& task, framework->completedTasks) {
            src//master/master.cpp:using memory::shared_ptr;
            src//master/master.cpp:  foreach (const shared_ptr<Framework>& framework, frameworks.completed) {
            src//master/master.cpp:  foreach (const shared_ptr<Framework>& framework, frameworks.completed) {
            src//master/master.cpp:  frameworks.completed.push_back(shared_ptr<Framework>(framework));
            src//master/master.hpp:    boost::circular_buffer<memory::shared_ptr<Framework> > completed;
            src//master/master.hpp:    completedTasks.push_back(memory::shared_ptr<Task>(new Task(task)));
            src//master/master.hpp:  boost::circular_buffer<memory::shared_ptr<Task> > completedTasks;
            src//slave/http.cpp:  foreach (const memory::shared_ptr<Task>& task, executor.completedTasks) {
            src//slave/slave.cpp:        foreach (const memory::shared_ptr<Task>& task,
            src//slave/slave.cpp:  completedTasks.push_back(memory::shared_ptr<Task>(task));
            src//slave/slave.hpp:  boost::circular_buffer<memory::shared_ptr<Task> > completedTasks;
            
            jieyu Jie Yu added a comment - Dominic, I think most of our shared_ptr usage in Mesos code base can be converted to unique_ptr. Yeah, we have lot of use of shared_ptr in libprocess. But that's for expert only I agree with you that unless we have a very strong reason to use shared_ptr, we should use process::Shared and Owned (unique_ptr) instead. [tw-mbp-jyu mesos (master)]$ grep -r shared_ptr src/ src//linux/routing/internal.hpp: memory::shared_ptr<Data> data; src//master/http.cpp: foreach (const memory::shared_ptr<Task>& task, framework.completedTasks) { src//master/http.cpp: foreach (const memory::shared_ptr<Framework>& framework, src//master/http.cpp: foreach (const memory::shared_ptr<Framework>& framework, src//master/http.cpp: foreach (const memory::shared_ptr<Task>& task, framework->completedTasks) { src//master/master.cpp:using memory::shared_ptr; src//master/master.cpp: foreach (const shared_ptr<Framework>& framework, frameworks.completed) { src//master/master.cpp: foreach (const shared_ptr<Framework>& framework, frameworks.completed) { src//master/master.cpp: frameworks.completed.push_back(shared_ptr<Framework>(framework)); src//master/master.hpp: boost::circular_buffer<memory::shared_ptr<Framework> > completed; src//master/master.hpp: completedTasks.push_back(memory::shared_ptr<Task>(new Task(task))); src//master/master.hpp: boost::circular_buffer<memory::shared_ptr<Task> > completedTasks; src//slave/http.cpp: foreach (const memory::shared_ptr<Task>& task, executor.completedTasks) { src//slave/slave.cpp: foreach (const memory::shared_ptr<Task>& task, src//slave/slave.cpp: completedTasks.push_back(memory::shared_ptr<Task>(task)); src//slave/slave.hpp: boost::circular_buffer<memory::shared_ptr<Task> > completedTasks;
            dhamon Dominic Hamon added a comment -

            I imagine that instead of using process::Shared and getting that safety, people will use process::Shared until they need mutability, and then will prefer std::shared_ptr anyway. We certainly have std::shared_ptr in the codebase so I don't think we're getting the safety you think we are.

            I just looked at the usage of both: process::Shared is used by process::Owned, the replicated log, and tests. std::shared_ptr is used everywhere.

            I'd like to reduce the complexity of the primitives available to use. As such, perhaps it is better to always prefer process::Shared over std::shared_ptr. What do you think?

            This would be unfortunate, given that we want to track modern C++ as closely as possible to encourage contributions from people, and the further we are from the standard, the harder that is. However, I agree that safety in our complex system is paramount.

            dhamon Dominic Hamon added a comment - I imagine that instead of using process::Shared and getting that safety, people will use process::Shared until they need mutability, and then will prefer std::shared_ptr anyway. We certainly have std::shared_ptr in the codebase so I don't think we're getting the safety you think we are. I just looked at the usage of both: process::Shared is used by process::Owned, the replicated log, and tests. std::shared_ptr is used everywhere. I'd like to reduce the complexity of the primitives available to use. As such, perhaps it is better to always prefer process::Shared over std::shared_ptr. What do you think? This would be unfortunate, given that we want to track modern C++ as closely as possible to encourage contributions from people, and the further we are from the standard, the harder that is. However, I agree that safety in our complex system is paramount.
            jieyu Jie Yu added a comment -

            In general, we do not encourage people to directly use std::shared_ptr in the code base (especially in Mesos code). The reason is because of the concurrency issue. If two threads both have the shared_ptr to the same shared data structure, and one of them tries to write the shared data structure, it could cause race condition. And in our actor based async programming environment, we usually disallow the use of mutex/locks as it could block worker threads! Therefore, we introduced Shared and it's read-only. In that way, race condition is not possible.

            jieyu Jie Yu added a comment - In general, we do not encourage people to directly use std::shared_ptr in the code base (especially in Mesos code). The reason is because of the concurrency issue. If two threads both have the shared_ptr to the same shared data structure, and one of them tries to write the shared data structure, it could cause race condition. And in our actor based async programming environment, we usually disallow the use of mutex/locks as it could block worker threads! Therefore, we introduced Shared and it's read-only. In that way, race condition is not possible.

            The benefit of only allowing const access is that we restrict the sharing of types to make them safer in our concurrent actor based environment! Forcing people to use std::shared_ptr<const T> is error prone, and it should be very rare (or we should avoid at all costs) using std::shared_ptr<T> without the const. Moreover, the ability to upgrade from Shared to Owned is highly useful in situations where you want to guarantee that something that was shared is now unique (and is already used in the code base). It's unclear if we'll ever get that kind of functionality from std::shared_ptr (where as converting a std::unique_ptr to a std::shared_ptr is trivial, hence it's existence).

            benjaminhindman Benjamin Hindman added a comment - The benefit of only allowing const access is that we restrict the sharing of types to make them safer in our concurrent actor based environment! Forcing people to use std::shared_ptr<const T> is error prone, and it should be very rare (or we should avoid at all costs) using std::shared_ptr<T> without the const. Moreover, the ability to upgrade from Shared to Owned is highly useful in situations where you want to guarantee that something that was shared is now unique (and is already used in the code base). It's unclear if we'll ever get that kind of functionality from std::shared_ptr (where as converting a std::unique_ptr to a std::shared_ptr is trivial, hence it's existence).
            dhamon Dominic Hamon added a comment -

            I don't understand the benefit of only allowing const access. If you want to store a std::shared_ptr to an object that can't be mutated, you should be able to use a std::shared_ptr<const T>.

            Owned will eventually be deprecated in favour of std::unique_ptr which already supports conversion to std::shared_ptr: http://www.cplusplus.com/reference/memory/shared_ptr/shared_ptr/

            dhamon Dominic Hamon added a comment - I don't understand the benefit of only allowing const access. If you want to store a std::shared_ptr to an object that can't be mutated, you should be able to use a std::shared_ptr<const T> . Owned will eventually be deprecated in favour of std::unique_ptr which already supports conversion to std::shared_ptr : http://www.cplusplus.com/reference/memory/shared_ptr/shared_ptr/
            jieyu Jie Yu added a comment -

            I am not sure how to deprecate this. Can you be more specific?

            Shared in process is a little different from std::shared_ptr:
            1) Shared only allows const access
            2) Shared can be converted to Owned

            jieyu Jie Yu added a comment - I am not sure how to deprecate this. Can you be more specific? Shared in process is a little different from std::shared_ptr: 1) Shared only allows const access 2) Shared can be converted to Owned

            People

              Unassigned Unassigned
              dhamon Dominic Hamon
              Dominic Hamon Dominic Hamon
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: