> On March 2, 2016, 2:58 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 37
> > <https://reviews.apache.org/r/44260/diff/1/?file=1276467#file1276467line37>
> >
> >     You don't really need to call `.self()` here, there exists an `defer` 
> > override taking process instance.
> 
> Benjamin Bannier wrote:
>     At least my clang does not trigger that overload.
> 
> Alexander Rukletsov wrote:
>     
> https://github.com/apache/mesos/blob/9bbba94021dde42c9d9d1fa0662462c364797018/3rdparty/libprocess/include/process/defer.hpp#L177
> 
> Benjamin Bannier wrote:
>     The issue I see is below when directly using the `Process` here. Note 
> that this works for code in `HierarchicalAllocatorProcess`.
>     
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:110:14: error: 
> no matching conversion for functional-style cast from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::function<Future<double> ()>'
>     ||       return std::function<R()>(f);
>     ||              ^~~~~~~~~~~~~~~~~~~~
>     || ../../src/master/allocator/mesos/metrics.cpp:38:9: note: in 
> instantiation of function template specialization 'process::_Deferred<double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()>::operator
>  Deferred<process::Future<double> >' requested here
>     ||         process::defer(
>     ||         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: 
> candidate template ignored: disabled by 'enable_if' [with _Fp = double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: 
> candidate constructor template not viable: requires 2 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : 
> __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: 
> candidate constructor template not viable: requires at least 3 arguments, but 
> 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: 
> candidate constructor not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:118:39: error: 
> no matching conversion for functional-style cast from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::function<Future<double> ()>'
>     ||           return dispatch(pid_.get(), std::function<R()>(f_));
>     ||                                       ^~~~~~~~~~~~~~~~~~~~~
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: 
> candidate template ignored: disabled by 'enable_if' [with _Fp = double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: 
> candidate constructor template not viable: requires 2 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : 
> __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: 
> candidate constructor template not viable: requires at least 3 arguments, but 
> 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: 
> candidate constructor not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || 1 warning and 2 errors generated.
>     || Error while processing 
> /XYZ/src/mesos/src/master/allocator/mesos/metrics.cpp.
>     || warning: 0.28.0": 'linker' input unused
>     || In file included from <built-in>:356:
>     || <command line>:4:24: warning: missing terminating '"' character 
> [-Winvalid-pp-token]
>     || #define PACKAGE_STRING "mesos
>     ||                        ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:110:14: error: 
> no matching conversion for functional-style cast from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::function<Future<double> ()>'
>     ||       return std::function<R()>(f);
>     ||              ^~~~~~~~~~~~~~~~~~~~
>     || ../../src/master/allocator/mesos/metrics.cpp:38:9: note: in 
> instantiation of function template specialization 'process::_Deferred<double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()>::operator
>  Deferred<process::Future<double> >' requested here
>     ||         process::defer(
>     ||         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: 
> candidate template ignored: disabled by 'enable_if' [with _Fp = double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: 
> candidate constructor template not viable: requires 2 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : 
> __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: 
> candidate constructor template not viable: requires at least 3 arguments, but 
> 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: 
> candidate constructor not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || In file included from ../../src/master/allocator/mesos/metrics.cpp:17:
>     || In file included from ../../src/master/allocator/mesos/metrics.hpp:26:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/metrics/gauge.hpp:19:
>     || In file included from 
> ../../3rdparty/libprocess/include/process/defer.hpp:19:
>     || ../../3rdparty/libprocess/include/process/deferred.hpp:118:39: error: 
> no matching conversion for functional-style cast from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::function<Future<double> ()>'
>     ||           return dispatch(pid_.get(), std::function<R()>(f_));
>     ||                                       ^~~~~~~~~~~~~~~~~~~~~
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1593:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'nullptr_t' for 1st argument
>     ||     function(nullptr_t) _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1594:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'const std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(const function&);
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1595:5: note: 
> candidate constructor not viable: no known conversion from 'double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*const)()'
>  to 'std::__1::function<process::Future<double> ()>' for 1st argument
>     ||     function(function&&) _NOEXCEPT;
>     ||     ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1599:41: note: 
> candidate template ignored: disabled by 'enable_if' [with _Fp = double 
> (mesos::internal::master::allocator::internal::HierarchicalAllocatorProcess::*)()]
>     ||                                         __callable<_Fp>::value &&
>     ||                                         ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1605:7: note: 
> candidate constructor template not viable: requires 2 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&) _NOEXCEPT : __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1608:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, nullptr_t) _NOEXCEPT : 
> __f_(0) {}
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1610:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, const function&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1612:7: note: 
> candidate constructor template not viable: requires 3 arguments, but 1 was 
> provided
>     ||       function(allocator_arg_t, const _Alloc&, function&&);
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1614:7: note: 
> candidate constructor template not viable: requires at least 3 arguments, but 
> 1 was provided
>     ||       function(allocator_arg_t, const _Alloc& __a, _Fp __f,
>     ||       ^
>     || /XYZ/src/llvm/P/bin/../include/c++/v1/functional:1591:5: note: 
> candidate constructor not viable: requires 0 arguments, but 1 was provided
>     ||     function() _NOEXCEPT : __f_(0) {}
>     ||     ^
>     || 1 warning and 2 errors generated.
>     || Error while processing 
> /XYZ/src/mesos/src/master/allocator/mesos/metrics.cpp.
> 
> Benjamin Bannier wrote:
>     Somehow this doesn't seem to work with `HierarchicalAllocatorProcess` the 
> way it does with e.g., `Master`. Leaving in the self for now, but definitely 
> worth investigating.
>     
>     Here's a reproducer:
>     
>     class P : public mesos::internal::master::allocator::MesosAllocatorProcess
>     {
>     public:
>       double hi() const { return 0; }
>       double ho() { return 0; }
>     };
>     
>     class M : public ProtobufProcess<M>
>     {
>     public:
>       double hi() { return 0; }
>     };
>     
>     void f(const P& p)
>     {
>       process::metrics::Gauge("defer via UPID and free function",
>                               process::defer(p, lambda::bind(&P::hi, &p)));
>     
>       // THIS DOES NOT WORK
>       // process::metrics::Gauge("defer via Process and member fct ptr",
>       //                         process::defer(p, P::hi)));
>     }
>     
>     void g(const M& m) {
>       // WORKS AS EXPECTED
>       process::metrics::Gauge("defer via Process and member fct ptr",
>                               process::defer(m, &M::hi));
>     }

A couple of things are going wrong here. Let's compare what's happening in the 
master metrics 
(https://github.com/apache/mesos/blob/master/src/master/metrics.cpp#L43) and 
allocator metrics.

The inheritance hierarchies of `Master` and `HierarchicalAllocatorProcess` are

    Master -> ProtobufProcess<Master> -> Process<M> -> ProcessBase
    HierarchicalAllocatorProcess -> MesosAllocatorProcess -> 
Process<MesosAllocatorProcess> -> ProcessBase

We also have `ProcessBase -> EventVisitor` but this is not relevant here.

The different `defer` overloads against which we could potentially match are 
(for some `typenames T, R`)

     (1) Deferred<void()> defer(const Process<T>& process, void (T::*method)())
     (2) Deferred<void()> defer(const Process<T>* process, void (T::*method)())
     (3) Deferred<Future<R>()> defer(const PID<T>& pid, Future<R> 
(T::*method)())
     (4) Deferred<Future<R>()> defer(const Process<T>& process, Future<R> 
(T::*method)())
     (5) Deferred<void()> defer(const PID<T>& pid, void (T::*method)())
     (6) Deferred<Future<R>()> defer(const PID<T>& pid, R (T::*method)())
     (7) Deferred<Future<R>()> defer(const Process<T>& process, R 
(T::*method)())
     (8) Deferred<Future<R>()> defer(const Process<T>* process, R 
(T::*method)())
     (9) _Deferred<Future<R>()> defer(const UPID& pid, F&& f)
    (10) Deferred<Future<R>()> defer(const Process<T>* process, Future<R> 
(T::*method)())

For the metrics use case here we are interrested only in methods returning a 
value, so overloads (1), (2) and (5) are not interesting for us.


`defer` resolution with `Master`
--------------------------------

When we `defer` a call to say `Master::_uptime_secs` (returning a `double`)

   const Master& master;
   defer(master, &M::_uptime_secs);

we could match against overloads (4), (7), or (9). We can match against (4) or 
(7) since when resolving we see e.g.,

    defer(const Process<Master>&, double (Master::*method);

where even though a derived to base cast is performed we still retain `Master` 
and can verify that `_uptime_secs` is a suitable `Master` member function.
We can match against (9) since a `UPID` can implicitly be constructed from a 
`ProcessBase` which also is just a derived to base cast away from `Master`.

Ultimatively (7) is choosen since it requires less conversions.


`defer` resolution with `HierarchicalAllocatorProcess`
------------------------------------------------------

When we `defer` a call to e.g., 
`HierarchicalAllocatorProcess::_event_queue_dispatches` derived to base casts 
leave us with a `Process<MesosAllocatorProcess>` which does not have the member 
function we are interested in. This excludes (4) and (7) from the matching set, 
so the only remaining match is (9). When expanding this match further we get 
the failure we are seeing due to peculiarities of `_Deferred` as opposed to 
`Deferred` since a `_Deferred` can only be used to call free functions, and not 
member functions.

Using `self()` (which is overridden for `HierarchicalAllocatorProcess`) made 
this work since we effectively perform a cast to 
`PID<HierarchicalAllocatorProcess>` and can match against overloads returning 
`Deferred` instead of `_Deferred`.


Method `const`ness in overload resolution
-----------------------------------------

Independently, the matter here is made even worse by the fact that I later 
declared the new getters in `HierarchicalAllocatorProcess` (e.g., 
`HierarchicalAllocatorProcess::_offer_filters`) as `const`. Since there is not 
`defer` overload for `const` member functions, none of the overloads taking 
`Process<T>` or `PID<T>` can match, and only the overload taking a `UPID` and a 
free function can match. This did work here since I used lambdas to wrap the 
member function calls (which is just the creation of free functions).


Summary
-------

In summary, when working with member function calls, the provided overloads of 
`defer` are pretty restrictive:

* the overloads taking `Process<T>` and `R (T::*method)` require matching types 
in the first and second argument for the match to be included. It should in 
principle be possible to declare a more permissive overload taking a 
`Process<U>` and a `R (V::*method)` and only later decide whether `U` and `V` 
are compatible.
* `defer`ing `const` member calls is overly cumbersome. A user should not feel 
tempted to declare effectively `const` methods mutable, only to satisfy an 
overload in a low-level library (here: `defer`).


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44260/#review121638
-----------------------------------------------------------


On March 3, 2016, 5:17 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44260/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
>     https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Moved metrics of the hierarchical allocator to its own file.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44260/diff/
> 
> 
> Testing
> -------
> 
> `make distcheck` on OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to