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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
