[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:530-560
+  struct loader {
+    DYLDRendezvous::iterator I;
+    ModuleSP m;
+    std::shared_future<void> f;
+  };
+  std::vector<loader> loaders(num_to_load);
+  llvm::ThreadPool load_pool(
----------------
scott.smith wrote:
> zturner wrote:
> > zturner wrote:
> > > I'm still unclear why we're still going down this route of adding a very specialized instance of parallelism to this one place, that is probably going to be less efficient than having a global thread pool (as now you will have to spin up and destroy the entire thread pool every time you go to load shared libraries).
> > > 
> > > So, rather than keep repeating the same thing over and over, I went and looked into this a little bit.  It turns out LLD has a large library of parallel algorithms in `lld/Include/lld/Parallel.h`.  There is almost nothing specific to LLD in any of these algorithms however, and they should probably be moved to llvm support.  Upon doing so, you will be able to write this code as:
> > > 
> > > ```
> > > std::vector<ModuleSP> Modules(m_rendevous.size());
> > > llvm::parallel_for_each(make_integer_range(0, m_rendevous.size()), [this, &Modules](int I)
> > >   {
> > >     auto &R = m_rendevous[I];
> > >     Modules[I] = LoadModuleAtAddress(R.file_spec, R.link_addr, R.base_addr, true);
> > >   });
> > > for (const auto &M : Modules)
> > >    module_list.Append(M);
> > > ```
> > > 
> > > Please look into making this change.
> > Note that this is, of course, exactly why I've been saying all along that we should not even be discussing this here.  Had the discussion happened over on the LLVM side the first time I asked about this, someone could have mentioned this without me having to spend time looking into it.
> As long as those routines allow you to recursively call those routines, then great, go for it.
> 
> The spinning up/tearing down of threads is not expensive, at least in my measured use case.
> As long as those routines allow you to recursively call those routines, then great, go for it.

Right, which is why I've asked at least 4 times for it to be looked into.  That doesn't mean mean use the first thing you see and check it in right away, it means please see if there is some way to use existing code to solve the problem.  If it turns out this doesn't allow recursive use, then a followup would be to understand why not, and if it is hard to change.

So again, please look into this and see if it will work for your needs.


Repository:
  rL LLVM

https://reviews.llvm.org/D32597