<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Mar 3, 2015 at 10:51 AM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Tue, Mar 3, 2015 at 8:53 AM, Shankar Kalpathi Easwaran <span dir="ltr"><<a href="mailto:shankarke@gmail.com" target="_blank">shankarke@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br>
Comment at: include/lld/Core/Parallel.h:77<br>
@@ +76,3 @@<br>
+/// <a href="https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01389.html" target="_blank">https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01389.html</a><br>
+template<typename T> class Future {<br>
+public:<br>
----------------<br>
there is only one usage of Future with lld::File, Why do we want to make this a template ?</blockquote><div><br></div></span><div>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.</div><div><br></div><div>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.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/ReaderWriter/FileArchive.cpp:63-69<br>
@@ -62,8 +62,9 @@<br>
{<br>
std::lock_guard<std::mutex> lock(_mutex);<br>
auto it = _preloaded.find(memberStart);<br>
if (it != _preloaded.end()) {<br>
- std::future<const File *> &future = it->second;<br>
- return future.get();<br>
+ std::unique_ptr<Future<const File *>> &p = it->second;<br>
+ Future<const File *> *future = p.get();<br>
+ return future->get();<br>
}<br>
}<br>
----------------<br>
The _preloaded map could get resized here ? Can we resize the map in advance since we know how many members are in the archive ?<br></blockquote><div><br></div></span><div>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.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/ReaderWriter/FileArchive.cpp:261-262<br>
@@ -263,4 +260,4 @@<br>
std::unique_ptr<Archive> _archive;<br>
mutable MemberMap _symbolMemberMap;<br>
mutable InstantiatedSet _membersInstantiated;<br>
atom_collection_vector<DefinedAtom> _definedAtoms;<br>
----------------<br>
looks like/most of the members are mutable, I think this needs to be made non-const.<br></blockquote><div><br></div></span><div>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.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/ReaderWriter/FileArchive.cpp:269<br>
@@ -271,4 +268,3 @@<br>
mutable std::vector<std::unique_ptr<MemoryBuffer>> _memberBuffers;<br>
- mutable std::map<const char *, std::future<const File *>> _preloaded;<br>
- mutable std::vector<std::unique_ptr<std::promise<const File *>>> _promises;<br>
+ mutable std::map<const char *, std::unique_ptr<Future<const File *>>> _preloaded;<br>
mutable std::mutex _mutex;<br>
----------------<br>
llvm::DenseMap ?</blockquote><div><br></div></span><div>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.</div></div></div></div>
</blockquote></div><br></div><div class="gmail_extra">Even Sanjoy's patch *doesn't*</div></div>