[PATCH] D113857: [llvm-reduce] Add parallel chunk processing.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I am writing lots of suggestion for something you may have intended to be a simple addition that could be improved later just as well. If this applies, please tell me.



================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.cpp:266
+
+    SmallVector<SmallString<0>> Results(NumJobs * 3, {});
+    for (auto I = ChunksStillConsideredInteresting.rbegin(),
----------------
fhahn wrote:
> Meinersbur wrote:
> > [serious] `Results` is never resized to cover the last `NumChunksProcessed`, i.e. we get a buffer overflow. Instead, we could use a container that never invalidates its elements such as `std:deque` or use a circular buffer.
> > 
> > Alternatively, you can change `TaskQueue` to `std::dequeue<std::pair<std::shared_future<void>, ResultTy>>` to the task and its result in the same queue.
> I think the code should only add new jobs if `NumChunkProcessed < Results.size()` in one iteration of the main loop, but there might be a an error. Adding it the to dequeue sounds like a great idea though to make things a bit simpler.
> 
> Alternatively we could also try to communicate the result through the `Future` directly, although that would require some changes to `ThreadPool`, because currently it just supports returning `shared_future<void>`.
Sorry, I did not see the `NumChunkProcessed < Results.size()` condition. IIUC, this will require the task queue to be cleared every `NumJobs * 3` jobs. I did not see that and could be avoided using the solutions that I mentioned.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.cpp:306
+        //  * we have not reached the end of the chunk.
+        while (!TaskQueue.empty()) {
+          auto &F = TaskQueue.front();
----------------
fhahn wrote:
> Meinersbur wrote:
> > This waits until the task queue is empty. Could we leave earlier as soon as a reduced version is available, and just forget the jobs not yet finished while scheduling the next ones?
> The current approach waits for each task, until it finds the first one that leads to a reduction. 
> 
> 
> It intentionally does not try to stop once any job reduces a chunk, because if we pick any reduced chunk we might get different results than during serial reduction.
Assume the current content of TaskQueue

0: job finished, no valid reduction
1: job still running
2: job finished, valid reduction result
3: job still running

I understood that the intention of AnyReduced is to not queue additional jobs (assuming `NumJobs > 4`) since we know that when hitting 2, we will have a reduced result. We still need to wait for job 1 which may also compute a valid reduction.
I think this is clever. I had `std::future<ResultTy>` in mind as you proposed yourself, until I discovered that `llvm::ThreadPool` only supports a void return type :-(. However, to avoid submitting more jobs after we know that there will a result available, I think a `AnyReduced` would still be useful flag.

My comment was regarding index 3 in the queue. Assuming job 1 finishes and job 2 becomes the front of the queue, do we still need to wait for job 3 before submitting jobs based on the new intermediate result? It may cause issues with who becomes responsible to free resources, so I am not sure its feasible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113857/new/

https://reviews.llvm.org/D113857