[PATCH] D53975: Start adding the supporting code to perform out-of-process allocator enumeration.

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In https://reviews.llvm.org/D53975#1284314, @kcc wrote:

> OMG.
>
> I sympathize with your problem, but the solution makes the code unreadable and unmaintainable. 
>  The point of having templates is to avoid code like
>
>   if (ObjectView::IsLocal()) {


That's not what the `ObjectView` (the template parameter) type does. The `ObjectView` abstraction let's its clients **look** at an object regardless of whether it exists in the current process or a remote process. The abstraction **does not abstract away operations** on the object.
It is the Object's job (in this case the allocator) to know how to perform operations on itself, whether that be in-process or out-of-process. This implies that the allocator has to handle performing operations on itself and this includes out-of-process operations. Trying to push
the knowledge of the peculiarities of a particular allocator into `ObjectView` type does not make sense from a software engineering perspective.

However there are things we could do to make this more aesthetically pleasing. For example we could make the `EnsureSortedChunks()` be a no-op when the allocator is instantiated with the `ObjectView` type for out-of-process enumeration.
Fundamentally however, the code path of in-process and out-of-process enumeration are similar but there are some divergences **that have to be handled somewhere** which is why you are seeing this `ObjectView::IsLocal()` guard.

> Also, if the change requires replacing
> 
>   auto t = chunks_[i];
> 
> with the code below I can not accept such a change.
> 
>   Header **header_ptr_addr = &(chunks_[i]);
>   // Copy header pointer into this process
>   typename ObjectView::template SmallAllocTy<Header *> header_ptr_view(
>       header_ptr_addr);
>   Header *t = *(header_ptr_view.GetLocalAddress());
>    
> 
> Is there a reasonable way to replace it with this?
> 
>   auto t = Load(&chunks_[i]);

Yes, but with a caveat. The type of `t` will not be `Header*` it will be `LocalObjectView<Header*>` for the in-process instantiation and `RemoteObjectView<Header*, StackStorage>` for the out-of-process.
The reason for this is that for the out-of-process case we need to manage the memory (allocate, copy into, and deallocate) for the object that is copied from a remote process. This is what the `RemoteObjectView`
object (not included in this patch) is doing.

However what we can do it provide an implicit conversion (like we do with `llvm::cl::opt<int>` today) so that can be treated as the type it represents (`Header*` in this case). Personally I'd prefer not to do the implicit conversion operation
because I prefer the programmer to be aware of the type they are working with (i.e. an `ObjectView<Header*>` and not a `Header*`) but I can see the appeal of the implicit conversion.

I'll make the change and let you take a look.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D53975