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

[Rui Ueyama]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150303/17e04631/attachment.html>