[PATCH] D34838: Prototype: Reduce llvm-profdata merge memory usage further

[Xinliang David Li via llvm-commits]

On Thu, Jun 29, 2017 at 5:29 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Jun 29, 2017 at 5:18 PM David Li via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> davidxl added a comment.
>>
>> Instead of exposing Counter members widely (and fixing tests), it is
>> better to add wrapper methods in InstrProfRecord to forward the call to the
>> nested member struct.
>>
>
> Seems like a lot of wrappers - is that the ideal API going forward, or
> only to minimize changes made by this patch? (I've already done the work &
> LLVM as a community's usually fine with the churn such that minimizing
> change isn't too important if it seems like the way things should be)
>
> Though I admit this is a bit so-so/uncertain. One pro is that I think
> lacking the wrappers encourages APIs to only depend on the part of the
> structure they need (in fact on that basis I'd consider sinking many of the
> value-profiling based functions into a separate class wrapping the
> unique_ptr) rather than the whole thing, making their contract clearer
> ("this function is only dealing with value profiling data", "this function
> cares about the counters as well", "this function needs names, etc") & more
> flexible (you can call it when you only have that part/chunk/thing, not
> needing the whole thing around).
>

Exposing the internals of the data structure in interfaces make future data
structure changes more difficult.  I understand there are quite a few
wrappers, but they are mostly just single-liners. With the wrappers
available, I expect most of the client side code remain unchanged (other
than the writer code).

David


>
>
>>
>>
>> https://reviews.llvm.org/D34838
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170629/eeb0a48f/attachment.html>