[PATCH] LLD: Implement our own future and use that for FileArchive::preload().

[Rui Ueyama]

On Tue, Mar 3, 2015 at 10:51 AM, Rui Ueyama <ruiu at google.com> wrote:

> On Tue, Mar 3, 2015 at 8:53 AM, Shankar Kalpathi Easwaran <
> shankarke at gmail.com> wrote:
>
>> ================
>> Comment at: include/lld/Core/Parallel.h:77
>> @@ +76,3 @@
>> +/// https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01389.html
>> +template<typename T> class Future {
>> +public:
>> ----------------
>> there is only one usage of Future with lld::File, Why do we want to make
>> this a template ?
>
>
> I didn't plan to implement a future. I first tried to do that all within
> FileArchive using mutex and condition variables, and then realized that
> that's too low-level multi-thread programming which is pretty error-prone.
>
> If you are familiar with multi-thread programming, futures are basic
> building block of high-level multi-thread programming. It's a rendezvous
> point of a producer thread and a consumer thread. I built a basic block
> here. Replacing T of this Future class with File * is weird.
>
>
>> ================
>> Comment at: lib/ReaderWriter/FileArchive.cpp:63-69
>> @@ -62,8 +62,9 @@
>>      {
>>        std::lock_guard<std::mutex> lock(_mutex);
>>        auto it = _preloaded.find(memberStart);
>>        if (it != _preloaded.end()) {
>> -        std::future<const File *> &future = it->second;
>> -        return future.get();
>> +        std::unique_ptr<Future<const File *>> &p = it->second;
>> +        Future<const File *> *future = p.get();
>> +        return future->get();
>>        }
>>      }
>> ----------------
>> The _preloaded map could get resized here ? Can we resize the map in
>> advance since we know how many members are in the archive ?
>>
>
> There's no such function like resize defined for std::map because of the
> nature of the map. It's a balanced tree, there's no such notion like making
> room for future use.
>
> ================
>> Comment at: lib/ReaderWriter/FileArchive.cpp:261-262
>> @@ -263,4 +260,4 @@
>>    std::unique_ptr<Archive> _archive;
>>    mutable MemberMap _symbolMemberMap;
>>    mutable InstantiatedSet _membersInstantiated;
>>    atom_collection_vector<DefinedAtom> _definedAtoms;
>> ----------------
>> looks like/most of the members are mutable, I think this needs to be made
>> non-const.
>>
>
> I agree. I actually tried to do that before. It currently needs to be
> const because Resolver.cpp uses all files as const objects. We need to
> remove consts from there.
>
>
>> ================
>> Comment at: lib/ReaderWriter/FileArchive.cpp:269
>> @@ -271,4 +268,3 @@
>>    mutable std::vector<std::unique_ptr<MemoryBuffer>> _memberBuffers;
>> -  mutable std::map<const char *, std::future<const File *>> _preloaded;
>> -  mutable std::vector<std::unique_ptr<std::promise<const File *>>>
>> _promises;
>> +  mutable std::map<const char *, std::unique_ptr<Future<const File *>>>
>> _preloaded;
>>    mutable std::mutex _mutex;
>> ----------------
>> llvm::DenseMap ?
>
>
> We don't have to use DenseMap here. The number of elements inserted to
> these maps are limited. DenseMap is less-understood semantics as we
> recently seen (which caused many subtle bugs). Even Sanjoy's patch catches
> all wrong uses of invalid references. If std::map just works fine, we don't
> have to replace it.
>

Even Sanjoy's patch *doesn't*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150303/5101a043/attachment.html>