Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
186 stars 47 forks source link

V2.1 idea: Should we switch to a functor upcall model, instead of our current method upcalls? #178

Closed KenBirman closed 4 years ago

KenBirman commented 4 years ago

A functor object is simply a C++ object that defines some standard upcall API, such as "invoke". These objects can have names, be templated, etc. So, in all ways, they can be used like methods. I want to suggest that in Derecho V 2.1 we switch to registering upcalls in our Derecho groups as functors rather than as methods.

This responds to an issue Weijia has run into several times now with new developers using Derecho/Cascade: they get confused and do p2p calls to endpoints intended as multicast endpoints, or vice versa. We have that issue because in Derecho, endpoints are identified by tags, which are than associated to the upcall method, but there is no compile time or runtime way to check that the endpoint type is actually compatible with the form of operation that was done. In V2.1 I want to unify everything under a single derecho::send API, but that will just make this kind of mistake even more likely.

If we switch to a functor model, we can offer two preexisting interface classes, both defining "invoke". One class would be derecho::p2p, and the other class, derecho::multicast. But because this provides us with the ability to define instance-specific constants, we would then have an easily accessible form of "endpoint type" information that can be accessed both at compile time or, if needed, at runtime. In fact I think the compile-time check this would enable might actually be adequate: I am thinking that I might be able to generate a constexpr table that can be indexed by endpoint tag, and would return the endpoint type. Then my generic send would have a compile time (templated) bit of logic asserting that the type is compatible with the invocation pattern (this derecho::send will be overloaded and just by the number of arguments, I can tell if it is a p2p operation or a multicast: the target of a p2p operation needs to be a specific member of a designated shard; the multicast version just specifies the shard, or omits it in which case "this" shard will be the target). Even if I can't do this via a constexpr table, we can easily test when a derecho::send is issued (and then no need to test again on delivery, since no illegal deliveries could occur!)

Does anyone see any issues with this? We can deprecate the old style of registering a method, but still support it, and then eliminate it entirely whenever we do a v3.0 release down the road.

mpmilano commented 4 years ago

I definitely see the reason you'd want to have specific endpoints behave as only P2P or only ordered, but I'm not sure what the exact functor proposal is. Right now each subgroup-backing class may register any number of methods as remote invocation targets. It would be straightforward enough to, at the point of registration, ask the programmer to specify whether a particular method should be invocable only P2P, only ordered, or both.

Is the purpose behind this proposal entirely motivated by the need to differentiate send targets? If so, this would have the desired effect and be the easier change to make.

KenBirman commented 4 years ago

Well, yes, this is exactly the goal. But I'm not seeing how we would implement and enforce that restriction. Would we need to have two distinct lists of tags -- one for p2p and one for multicast?

mpmilano commented 4 years ago

Basically! We'd replace our single registration macro with two different registration macros users could call - one to register P2P, the other to register send. Internally, the templates manipulate the tag, prepending a bit to it (0 or 1 for p2p or ordered). The corresponding invocation template will similarly prepend the relevant bit to the user-provided tag, ensuring that it can only match methods registered via the corresponding macro.

KenBirman commented 4 years ago

Very simple! I guess I would already favor just doing that in V2.1. Weijia wasted some time on people making this mistake, and none of us really has time for that sort of thing.

mpmilano commented 4 years ago

I'm writing up some code and a set of changes that we'd need to do to implement this. I'm going to take this opportunity to clean up register_functions.hpp, Which is currently a couple hundred lines of copypasta due to limitations in C macro vararg processing that we didn't know how to get around at the time.

mpmilano commented 4 years ago

Wow those macros were much harder to work with than I expected! I've attached a zip file with a new version of the REGISTER_RPC_FUNCTIONS macro which works like this:

REGISTER_RPC_FUNCTIONS(classname, ORDERED_TARGETS(methods...), P2P_TARGETS(methods...))

the ORDERED_TARGETS and P2P_TARGETS can come in either order, and either (or both) may be omitted. The compiler will check to make sure you've actually passed ORDERED_TARGETS or P2P_TARGETS to the REGISTER_RPC_FUNCTIONS macro, thus catching (and flagging as an error) any code which just passes method names straight to REGISTER_RPC_FUNCTIONS without a tag.

This isn't the sum total of necessary changes, though. The old macros worked by emitting a request to tag each method. The new ones work by emitting a request to tag_ordered or tag_p2p those methods, as appropriate. Thus we also need to replace the tag logic with new tag_ordered and tag_p2p variants. (Or we could add another template parameter to the existing tag method indicating whether the request is for ordered or p2p registration).

The two tag functions starting on line 458 of remote_invocable.hpp need to be changed to allow this, either replacing the single tag methods with a tag_ordered and tag_p2p, or introducing a new template argument to differentiate ordered from p2p. In this file the first existing tag is for non-const functions, and the second is for const functions; it's possible we only want to implement tag_p2p for the const functions.

Depending on how deep you want to go, there are two possible paths from here. One is to just manipulate the FunctionTag (which is just an unsigned long long calculated from "hashing" the user-provided tag) itself, doubling it in the case of ordered_send and doubling it and adding one in the case of p2p_send. This thus produces the invariant that "even" function tags are for ordered send, and "odd' ones are for p2p_send.

If you choose this route, the only other change would be to the various definitions of ordered and p2p sends found in replicated_impl.hpp, starting on line 76, where you'd need to do the same mathematical transformation to the FunctionTags here.

The other route would be to actually split the FunctionTag type itself into an OrderedFunctionTag and a P2PFunctionTag. To do this, you'd need to modify the wrapped and partial_wrapped structs in remote_invocable.hpp to include a parameter indicating whether the tag is for ordered or p2p.

I'm happy to walk someone through these changes, if there's anyone who wants to take actual-code-modification responsibilities on. I've sadly run out of time to pursue these changes myself for the day --- and my next solid chunk of uncommitted work time isn't likely to be until close to Thanksgiving.

mpmilano commented 4 years ago

Apparently I can't add attachments, sorry --- zip file here

etremel commented 4 years ago

Nice work, Matthew. It's great that the hundreds of lines of REGISTER_RPC_FUNCTIONS variants for different numbers of arguments can be replaced with a much shorter EVAL(MAP()) macro. I'm also pleasantly surprised to learn that there is now a way to write a compile-time string hash function without needing the CT_STRING macro and rpc::String<char...> type.

I think it would be pretty easy for me to integrate these new macros with the tag functions in remote_invocable.hpp, since I'm already intimately familiar with the RemoteInvocable and Replicated class implementations. I can start doing this in a new branch, although to reduce complications, I would rather not merge it until my signed_log_support branch has been merged (see pull request #179) - I wouldn't want to introduce merge conflicts into that branch.

mpmilano commented 4 years ago

Oh awesome, yeah I see no reason this can't wait until your incoming is merged. Happy to hop on a call after next week to go over the changes and such. Worth noting that the "hash" function in the demo code probably doesn't match the behavior of the one we're using now, so that's another change to investigate.

songweijia commented 4 years ago

This is a super cool feature to avoid blending p2p_send and ordered_send! So that we can separate the p2p calls and ordered calls. When we have it ready, most of the applications with cooked p2p send is going to change. And I agree with Edward that we need to merge the pending pull request (with the signed feature) on everyone's agreement. Edward and I had evaluated the performance separately. The original performance is not affected by the new feature.

Best Regards, Weijia Song Department of Computer Science Cornell University

On Fri, Nov 13, 2020 at 6:59 PM Matthew Milano notifications@github.com wrote:

Oh awesome, yeah I see no reason this can't wait until your incoming is merged. Happy to hop on a call after next week to go over the changes and such. Worth noting that the "hash" function in the demo code probably doesn't match the behavior of the one we're using now, so that's another change to investigate.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Derecho-Project/derecho/issues/178#issuecomment-727091751, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACF4HXOQEOT6XLVY4SGR623SPXB5XANCNFSM4TSA6RGA .

mpmilano commented 4 years ago

Implementation is here: https://github.com/Derecho-Project/derecho/tree/tagged_p2p_functions and looks like it's working so far. Edward wanted to do a little more testing before merging it though. In the process of integrating this Edward also noticed that Persistent isn't const-correct; @songweijia is there something blocking const-labelled methods for reading persistent state, or is it just a matter of doing the const-copypasta? Once persistent is const-correct we can enforce, at compile-time, that all P2P methods are const, which in turn enforces that replicas within a subgroup are mutated only via the ordered_send.

etremel commented 3 years ago

Update for anyone watching this thread: Persistent now has const labelling on its getter methods, and tagged_p2p_functions is ready to merge into master. However, it's still not possible to enforce at compile-time that all P2P-tagged methods are const, because a P2P-tagged method might legitimately want to modify some local non-replicated state on the object (i.e. fields that are not serialized), such as a random number generator or a logger. So the new P2P-tagging functions will allow you to tag a non-const method as P2P-callable, but users should really try to make all P2P-callable methods const unless the state they are modifying is not in a serialized (replicated) field.

mpmilano commented 3 years ago

Quick question: why not just mark the non-serialized fields as mutable, a keyword which allows const-annotated methods to mutate them? It seems like the cases you're outlining are precisely what mutable was designed for: when the interior of an object mutates in a way that isn't readily visible to callers of that objects' API.