[PATCH] D124715: Use QoS class Utility for ThreadPriority::Low on Mac

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks a lot, this version looks good to me and I'll follow up with a flag for clangd.

In D124715#3493493 <https://reviews.llvm.org/D124715#3493493>, @stefanhaller wrote:

> In D124715#3491985 <https://reviews.llvm.org/D124715#3491985>, @sammccall wrote:
>
>> I'm still concerned some users will find this a large regression and we won't have a good workaround:
>>
>> - it'll use a lot more battery than before
>
> On Intel Macs, I'm not sure that's true. While it does saturate the CPUs noticeably more with Utility than it does with Background, it will also be finished much quicker, so I guess the total power consumption will probably be roughly the same.

Yes, the issue is with workloads where it doesn't finish during the editor session, e.g. because indexing the full project takes hours.



================
Comment at: clang/tools/libclang/CIndex.cpp:9028
 #if LLVM_ENABLE_THREADS
-  llvm::set_thread_priority(llvm::ThreadPriority::Background);
+  llvm::set_thread_priority(llvm::ThreadPriority::Low);
 #endif
----------------
There's a mismatch between name and behavior here.

Could you add a comment explaining that setThreadBackgroundPriority name is for historical reasons, but Low is more appropriate?


================
Comment at: llvm/include/llvm/Support/Threading.h:236
   enum class ThreadPriority {
+    /// Try to lower current thread's priority as much as possible. Can be used
+    /// for long-running tasks that are not time critical; more energy-efficient
----------------
nit: I'd drop "try to" from each of these. 
The failure modes are to do with set_thread_priority rather than the enum, so the comment belongs there if at all (I think the return type covers it though).


================
Comment at: llvm/lib/Support/Unix/Threading.inc:259
   // SCHED_IDLE    for running very low priority background jobs.
   // SCHED_OTHER   the standard round-robin time-sharing policy;
   return !pthread_setschedparam(
----------------
Maybe add a FIXME: consider SCHED_BATCH for Low?


================
Comment at: llvm/lib/Support/Windows/Threading.inc:122
   // priorities of the thread as they were before the thread entered background
   // processing mode.
   return SetThreadPriority(GetCurrentThread(),
----------------
maybe add a FIXME: consider THREAD_PRIORITY_BELOW_NORMAL for Low?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124715