[PATCH] D24622: LTO: Simplify caching interface.

[Teresa Johnson via llvm-commits]

On Thu, Sep 22, 2016 at 11:48 PM, Peter Collingbourne <peter at pcc.me.uk>
wrote:

> What do you think about the below interface changes? It seems like they
> would address most of the concerns you're raising here.
>
> typedef std::function<AddStreamFn(unsigned Task, StringRef Key)>
> NativeObjectCache;
>
> NativeObjectCache localCache(std::string CacheDirectoryPath, AddFileFn
> AddFile);
>
> Error LTO::run(AddStreamFn AddStream, NativeObjectCache Cache)
>
> And remove Cache field from config.
>

Can't speak for Mehdi, but I do like this change - it makes it much clearer
that the AddFile is correlated with the local cache implementation, and
doesn't require passing it in to LTO::run when e.g. we are doing regular
LTO and no caching. It also seems like it would facilitate supporting an
in-memory cache as Mehdi asked, since the NativeObjectCache isn't requring
the AddFileFn.

Teresa

Peter
>
> On Sep 22, 2016 10:25 PM, "Mehdi AMINI" <mehdi.amini at apple.com> wrote:
>
>> mehdi_amini added inline comments.
>>
>> ================
>> Comment at: include/llvm/LTO/Config.h:60
>> @@ +59,3 @@
>> +/// cache, pass a unique string as the Key. For hits, the cached file
>> will be
>> +/// added using AddFile and this function will return AddStreamFn(). For
>> misses,
>> +/// the cache will return a stream callback which must be called at most
>> once to
>> ----------------
>> The "AddFile" part seems convoluted to me. It seems to be here only to
>> satisfy the flow of having a stream as a returned type.
>>
>> ================
>> Comment at: include/llvm/LTO/Config.h:72
>> @@ -64,2 +71,3 @@
>> +    NativeObjectCache;
>>
>>  /// LTO configuration. A linker can configure LTO by setting fields in
>> this data
>> ----------------
>> Since you're claiming to make it easier to change the cache
>> implementation in the description, can you clarify how would I implement a
>> "in-memory" cache with this interface?
>>
>> ================
>> Comment at: include/llvm/LTO/Config.h:116
>> @@ -107,1 +115,3 @@
>>
>> +  NativeObjectCache Cache;
>> +
>> ----------------
>> No documented.
>>
>> ================
>> Comment at: include/llvm/LTO/LTO.h:316
>> @@ +315,3 @@
>> +  /// AddFile functions to add native object files to the link.
>> +  ///
>> +  /// The client will receive at most one callback (via either AddStream
>> or
>> ----------------
>> > Is there any specific client you have in mind where this would be a
>> problem?
>>
>> That's not the angle I'm looking at when looking at the API design.
>>
>> For instance:
>> Why is `NativeObjectCache Cache` in in the config? While these callbacks
>> are here?
>>
>>
>>
>> https://reviews.llvm.org/D24622
>>
>>
>>
>>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160923/8ffbc850/attachment.html>