[Xorp-hackers] More on XRL and Thrift.

[Bruce Simpson]

Ben Greear wrote:
>
> Xorp is a royal pain in the arse to read, with all it's typedefs, deep 
> class inheritance,
> auto-generated templated code (try to read the callback code some 
> day..impossible),
> timers, xrl black hole, chained (and unchained, for that matter) 
> callbacks, etc.
>
> It is one of the most hard to read pieces of code I've ever looked at 
> (only the original
> Vocal project was just barely worse, primarily because it was threaded 
> and full of bugs).
> Maybe Thrift will help..but if it's just yet another indirection or 
> black hole of
> magic, I doubt it.

XRL and Thrift bear some comparison.

    The one that sprang to mind just as I put my dinner on the hob, was 
this one: Thrift draws a clean distinction between the RPC transport, 
and the representation used on the transport. XRL does have such a 
distinction, but it isn't as clear cut for the transport, or the 
representation. So there is quite a bit of bleed-through across the code 
for each XrlPF.

In places this leads to some dire performance problems due to the level 
of indirection involved. Thrift, on the other hand, is pretty lean and 
mean in its C++ library.

    What I'll aim to do is to release parts of the Thrifted XORP tree 
I'm comfortable with and which could use further review.

    Earlier in this thread, we saw a situation which could only arise 
because in XRL, we attempt to cache a pointer to the transport endpoint, 
in the client-side RPC stub itself, with no way that pointer could be 
cleanly invalidated. Xrl's use of XrlPFSender here is strictly as a 
cache -- class Xrl does not participate in the life cycle of 
XrlPFSender, beyond being used by it.

    This is actually a really good use of a Boost weak_ptr. It is quite 
literally an observation of a shared_ptr. That pointer can happen to be 
invalid. But the situation arose in the first place because of XRL's 
granularity being per-method only, which is why I'd argue it's a design bug.

    But let's flip back to how callback stubs are generated, and end up 
in libfubarxif.so. The XrlFooClient object is instantiated. Whilst it's 
associated with an XrlRouter to begin with, this is in fact an 
association with the stereotype XrlSender (a thing which can send XRLs). 
We don't interact with this object much beyond invoking its send() 
method, when the client calls XrlFooClient::send_foo().

    Part of the problem here is that XRL attempts to do call resolution 
per-method.

    In a Thrifted world, the XrlSender can inform the XrlFooClient 
object that the endpoint changed, but this need only be on a per-service 
basis; there's no need to cache every single method call resolution, as 
the XRL foo_xif.cc stubs currently attempt to, and as you saw, this just 
caused problems when the endpoints themselves were possibly subject to a 
race.

    Broadly, in Thrift, the equivalent of that Xrl::resolved_sender() 
pointer is the output transport pointer. Because the transport is just 
something which can be written to, to issue an RPC call, we can deal 
with the semantics of moving to a new endpoint outside of the scope of 
that call.
    In fact, we may be best off providing the XORP apps a TMemoryBuffer 
to scribble the shimmed XRL calls into. This means a transport is always 
available.

    Because we're then dealing with a binary blob, rather than a local 
copy of an Xrl frankenblob, dispatching it is really easy, and we can 
cache the endpoint to our heart's content, probably using a 
boost::weak_ptr to boot.
    We can then let libxipc decide how to route it, in a runtime scope 
where we are more in control of the endpoint situation, rather than 
being at the mercy of a lone pointer.

>
> I think if I spent more than 2 days looking at XRL I'd rip it's guts 
> out and
> re-implement it entirely.  I don't envy your task, and I hope it works 
> out well.

No comment. ;-)

I am generally pleased with how it's going, and got much closer to the 
action this week.

It took a lot of reading to figure out that what is being attempted is 
in fact possible, but it's going to take a bit of effort to get it off 
the ground.

thanks,
BMS