scott.smith added a subscriber: ruiu.
scott.smith 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(
----------------
zturner wrote:
> 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.
It seems there are two possible long term fixes:
1. Separate thread pool
2. Make parallel_for support recursive use.
Personally I'm in favor of 2. @ruiu brought it up as a preferred goal too.
The q is then how to proceed in the short term:
1. Wait for parallel_for to land in llvm, make it support recursion (hopefully
Windows ConcRT supports it too?), then convert this change and commit.
2. Change TaskMapOverInt to take a ThreadPool, then have it use a private
ThreadPool for now, then easily convert to the #1 above later.
3. Change TaskMapOverInt to support recursion now (which will also require
changes to lldb::TaskPool, IMO), then do #1 above once it lands. Advantage is
we prove it works, which hopefully makes moving it upstream easier.
Any preferences on how to proceed?
Repository:
rL LLVM
https://reviews.llvm.org/D32597
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
