ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land.
LGTM (just one more possibly useful nit about const) ================ Comment at: clangd/TUScheduler.cpp:298 + while (shouldSkipHeadLocked()) + Requests.pop_front(); + assert(!Requests.empty() && "skipped the whole queue"); ---------------- ilya-biryukov wrote: > sammccall wrote: > > ilya-biryukov wrote: > > > Instead of skipping requests here we could try removing them from back of > > > the queue in `startTask` (or rewriting the last request instead of adding > > > a new one). > > > It feels the code could be simpler, as we will only ever have to remove a > > > single request from the queue. And it could also keep the queue smaller > > > in case of many subsequent `Auto` requests. > > > WDYT? > > Having startTask look ahead to find things to cancel was the thing I found > > most confusing/limiting in the previous code, so I'd rather not go back > > there :-) > > That said, I did try this first, trying to limit the scope of this patch, > > but it got hard. > > > > The main problems are: > > - you're not just looking ahead one task, or even to a fixed one. After > > [auto no], no cancels no, auto cancels both, read cancels neither. The > > states and the state machine are hard to reason about. (unless you just > > loop over the whole queue, which seems just as complex) > > - the decision of "should task X run" is distributed over time via mutating > > state, rather than happening at one point via reads > > - when looking at startTask time, you have to reason about the (possibly) > > concurrently running task. In run(), no task is running and nothing can be > > enqueued, so there's no concurrency issues. > > > > >And it could also keep the queue smaller in case of many subsequent Auto > > >requests. > > This is true, but it doesn't seem like a practical concern. > Thanks for clarifying. The first bullet point shouldn't be a big a problem. > Yes, the new task can remove multiple items from the back of the queue, but > the implementation still looks more natural as it only needs to inspect the > **last** item on the queue on each of the outer loop iterations. (While the > current implementation has to do an inner loop through multiple items on the > queue in addition to the outer loop). > > The second point makes it hard, though. I would probably go with calling > `pop_front()` when removing the request and signalling empty queue separately. > > > This is true, but it doesn't seem like a practical concern. > It isn't, but I still think it's a nice-to-have. As discussed offline, I totally missed the case when the new request comes in and has to cancel requests from the middle of the queue. So the implementation I proposed would probably still have two loops and not be less complex. So LGTM here, my suggestions won't make things simpler. ================ Comment at: clangd/TUScheduler.cpp:105 + /// Should the first task in the queue be skipped instead of run? + bool shouldSkipHeadLocked(); ---------------- NIT: this could be made `const` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
