Add ThreadPriority::Low, and use QoS class Utility on Mac

[stefanhaller]

Summary

On Apple Silicon Macs, using a Darwin thread priority of PRIO_DARWIN_BG seems to
map directly to the QoS class Background. With this priority, the thread is
confined to efficiency cores only, which makes background indexing take forever.

Introduce a new ThreadPriority "Low" that sits in the middle between Background
and Default, and maps to QoS class "Utility" on Mac. Make this new priority the
default for indexing. This makes the thread run on all cores, but still lowers
priority enough to keep the machine responsive, and not interfere with
user-initiated actions.

I didn't change the implementations for Windows and Linux; on these systems,
both ThreadPriority::Background and ThreadPriority::Low map to the same thread
priority. This could be changed as a followup (e.g. by using SCHED_BATCH for Low
on Linux).

See also https://github.com/clangd/clangd/issues/1119.

[dgoldman]

llvm/include/llvm/Support/Threading.h

249–250

Should be "Low" now

Wonder if this comment should be moved up into ThreadPriority?

[stefanhaller]

Fix documentation of ThreadPriority::Low (was: Background), and move it up from
set_thread_priority to ThreadPriority.

(Is it ok that there's no documentation left for set_thread_priority? I couldn't
come up with anything that's not trivial and redundant, e.g. "Sets the priority
of the current thread.")

[dgoldman]

In D124715#3484058, @stefanhaller wrote:

Fix documentation of ThreadPriority::Low (was: Background), and move it up from
set_thread_priority to ThreadPriority.

(Is it ok that there's no documentation left for set_thread_priority? I couldn't
come up with anything that's not trivial and redundant, e.g. "Sets the priority
of the current thread.")

LGTM, no need for a trivial comment, the priorities themselves are documented.

[sammccall]

With this priority, the thread is
confined to efficiency cores only, which makes background indexing take forever

(How) does this interact with battery vs mains power on laptops?
It seems like there's a common scenario where:

• the user is on a relatively slow laptop, running off battery
• the codebase is large, and indexing is unlikely to finish within an editing session

In this case, it seems like only using efficiency cores is what you'd want, and that people are likely to be upset if clangd 15 keeps their performance cores running at all times.

Reading the docs it seems like background is the intended QoS for this type of work ("such as indexing"..."minutes or hours").

(Sorry to bring this up at code review time, I wasn't thinking about laptops when we discussed on the bug)

[stefanhaller]

In D124715#3488447, @sammccall wrote:

(How) does this interact with battery vs mains power on laptops?
It seems like there's a common scenario where:

• the user is on a relatively slow laptop, running off battery
• the codebase is large, and indexing is unlikely to finish within an editing session

In this case, it seems like only using efficiency cores is what you'd want, and that people are likely to be upset if clangd 15 keeps their performance cores running at all times.

I can only speak for myself here: I'm a laptop user myself, and I work on battery the majority of the day; but I still wouldn't trade indexing speed for saving battery. The clangd index is so essential for my work that I always want it to be available as quickly as possible.

Reading the docs it seems like background is the intended QoS for this type of work ("such as indexing"..."minutes or hours").

How long it takes is only one aspect of it. Other clues from the documentation:

Utility: "typically have a progress bar that is visible to the user. Focuses on providing a balance between responsiveness, performance, and energy efficiency."
Background: "isn't visible to the user. Focuses on energy efficiency."

The way I read it, Background is intended for tasks that don't make a significant difference to the functionality of the software. An example might be face detection in the Photos app; as long as it is not finished, it only affects a small part of the functionality, the rest of the app is perfectly usable. That's not the case for clangd, the software is pretty much not usable without an index.

[dgoldman]

In D124715#3490364, @stefanhaller wrote:

In D124715#3488447, @sammccall wrote:

(How) does this interact with battery vs mains power on laptops?
It seems like there's a common scenario where:

• the user is on a relatively slow laptop, running off battery
• the codebase is large, and indexing is unlikely to finish within an editing session

In this case, it seems like only using efficiency cores is what you'd want, and that people are likely to be upset if clangd 15 keeps their performance cores running at all times.

I can only speak for myself here: I'm a laptop user myself, and I work on battery the majority of the day; but I still wouldn't trade indexing speed for saving battery. The clangd index is so essential for my work that I always want it to be available as quickly as possible.

Reading the docs it seems like background is the intended QoS for this type of work ("such as indexing"..."minutes or hours").

How long it takes is only one aspect of it. Other clues from the documentation:

Utility: "typically have a progress bar that is visible to the user. Focuses on providing a balance between responsiveness, performance, and energy efficiency."
Background: "isn't visible to the user. Focuses on energy efficiency."

The way I read it, Background is intended for tasks that don't make a significant difference to the functionality of the software. An example might be face detection in the Photos app; as long as it is not finished, it only affects a small part of the functionality, the rest of the app is perfectly usable. That's not the case for clangd, the software is pretty much not usable without an index.

See also Argyrios's comment - I think the main problem is it competes with Spotlight indexing and disk backups. Maybe we'd want to keep this customizable though e.g. via a clangd flag?

[akyrtzi]

My recommendation is that indexing belongs to 'utility', as @stefanhaller mentioned the user is actively depending on functionality coming from the index.
That said, you may want to consider dynamically switching to background if running on laptop with battery, or other heuristics, but that could be a follow-up enhancement, I don't think always using 'background' is appropriate.

[stefanhaller]

In D124715#3491782, @akyrtzi wrote:

That said, you may want to consider dynamically switching to background if running on laptop with battery, or other heuristics,

My take on this is that it would be Apple's responsibility to add a QoS class that has this behavior. I don't think it makes sense for every application to implement this logic client-side.

[sammccall]

OK, so wanting this to be utility makes sense, and maybe/probably it should be the default.

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
• not everyone will see the background index as so critical
• I think our only advice can be to disable background indexing entirely

What do you think about keeping Background in the enum along with Low, and switching clangd to Low?
I'm happy to do the plumbing to add a flag to clangd, and to implement Low for other OSes.
(Digging into windows, it looks like the background mode there is also much slower).

@kadircet any opinion here?

[stefanhaller]

In 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.

On M1 Macs, it's true that it will use a lot more battery. However, M1 Macs are also much more energy efficient in general, so it doesn't matter nearly as much. I don't think it will be perceived as a regression there.

(On the contrary, people who switch from Intel to M1 Macs perceive it as a regression that indexing is so slow, even though the machine should be so much faster. I have plenty of colleagues who are in that situation, and have heard this complaint several times.)

What do you think about keeping Background in the enum along with Low, and switching clangd to Low?

Personally I don't feel this is necessary, but I can of course change the patch if there's agreement that that's what we should do.

[kadircet]

I agree that clangd's indexing should probably be higher priority than whatever FS indexing is going on (because clangd's indexing only start when user actually starts working on a project, hence this indicates some level of "interactiveness"). so I feel like the right default is Utility, rather than Background.
But we had this behavior for a long while and people didn't complain up until M1s. So this is definitely going to result in some behavior change for intel users, even if we agree on this is making the state better for M1s.
Unfortunately I am not in a position to say what does the Utility vs Background imply for intel chips for sure, to tell the behavior change isn't going to be a regression. If this renders next clangd unusable by intel mac folks, it would be really unfortunate.

So I am also in favor of keeping the Background and switch the default in clangd to Low, while providing a flag to turn this back into Background. Hopefully we can retire the flag in between.

[stefanhaller]

Updated the patch to have both Background and Low

As discussed, we provide both Background and Low now, with Low being the
default; Linux and Windows still implement both the same (as Background), this
could be changed as a followup, as I didn't feel comfortable touching these.

[sammccall]

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

In D124715#3493493, @stefanhaller wrote:

In 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.

clang/tools/libclang/CIndex.cpp

9028–9030

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?

llvm/include/llvm/Support/Threading.h

236

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).

llvm/lib/Support/Unix/Threading.inc

259

Maybe add a FIXME: consider SCHED_BATCH for Low?

llvm/lib/Support/Windows/Threading.inc

122–124

maybe add a FIXME: consider THREAD_PRIORITY_BELOW_NORMAL for Low?

[stefanhaller]

• Added a comment to setThreadBackgroundPriority
• Improved comments for ThreadPriority enum
• Add FIXME comments for Windows and Linux

[stefanhaller]

@sammccall I addressed your last change requests with a revised patch, do you want to have a final look?

Also, I don't have commit rights, could somebody please land this for me once everybody is happy with it?

[sammccall]

Looks great! I'm happy to land it, will do so on Tuesday unless anyone has further comments.

[stefanhaller]

In D124715#3498846, @sammccall wrote:

Looks great! I'm happy to land it, will do so on Tuesday unless anyone has further comments.

@sammccall Just a friendly ping, in case you forgot. 😄

[sammccall]

In D124715#3510908, @stefanhaller wrote:

In D124715#3498846, @sammccall wrote:

Looks great! I'm happy to land it, will do so on Tuesday unless anyone has further comments.

@sammccall Just a friendly ping, in case you forgot. 😄

Sorry about that! Landed now :-)

clang-tools-extra/clangd/index/Background.h

 // A priority queue of tasks which can be run on (external) worker threads.
 class BackgroundQueue {
 public:
   /// A work item on the thread pool's queue.
   struct Task {
     explicit Task(std::function<void()> Run) : Run(std::move(Run)) {}
 
     std::function<void()> Run;
-    llvm::ThreadPriority ThreadPri = llvm::ThreadPriority::Background;
+    llvm::ThreadPriority ThreadPri = llvm::ThreadPriority::Low;
     unsigned QueuePri = 0; // Higher-priority tasks will run first.
     std::string Tag;       // Allows priority to be boosted later.
     uint64_t Key = 0;      // If the key matches a previous task, drop this one.
                            // (in practice this means we never reindex a file).
 
     bool operator<(const Task &O) const { return QueuePri < O.QueuePri; }
   };
 

clang/tools/libclang/CIndex.cpp

 
 } // namespace clang
 
 void clang::setThreadBackgroundPriority() {
   if (getenv("LIBCLANG_BGPRIO_DISABLE"))
     return;
 
 #if LLVM_ENABLE_THREADS
-  llvm::set_thread_priority(llvm::ThreadPriority::Background);
+  // The function name setThreadBackgroundPriority is for historical reasons;
+  // Low is more appropriate.
+  llvm::set_thread_priority(llvm::ThreadPriority::Low);
 #endif
 }
 
 void cxindex::printDiagsToStderr(ASTUnit *Unit) {
   if (!Unit)
     return;
 
   for (ASTUnit::stored_diag_iterator D = Unit->stored_diag_begin(),

llvm/include/llvm/Support/Threading.h

#endif
   /// group, the calling thread can be executed. On Windows, threads cannot
   /// cross CPU sockets boundaries.
   llvm::BitVector get_thread_affinity_mask();
 
   /// Returns how many physical CPUs or NUMA groups the system has.
   unsigned get_cpus();
 
   enum class ThreadPriority {
+    /// Lower the current thread's priority as much as possible. Can be used
+    /// for long-running tasks that are not time critical; more energy-
+    /// efficient than Low.
     Background = 0,
-    Default = 1,
+
+    /// Lower the current thread's priority such that it does not affect
+    /// foreground tasks significantly. This is a good default for long-
+    /// running, latency-insensitive tasks to make sure cpu is not hogged
+    /// by this task.
+    Low = 1,
+
+    /// Restore the current thread's priority to default scheduling priority.
+    Default = 2,
   };
-  /// If priority is Background tries to lower current threads priority such
-  /// that it does not affect foreground tasks significantly. Can be used for
-  /// long-running, latency-insensitive tasks to make sure cpu is not hogged by
-  /// this task.
-  /// If the priority is default tries to restore current threads priority to
-  /// default scheduling priority.
   enum class SetThreadPriorityResult { FAILURE, SUCCESS };
   SetThreadPriorityResult set_thread_priority(ThreadPriority Priority);
 }
 
 #endif

llvm/lib/Support/Unix/Threading.inc

 #include "Unix.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 
 #if defined(__APPLE__)
 #include <mach/mach_init.h>
 #include <mach/mach_port.h>
+#include <pthread/qos.h>
 #endif
 
 #include <pthread.h>
 
 #if defined(__FreeBSD__) || defined(__OpenBSD__)
 #include <pthread_np.h> // For pthread_getthreadid_np() / pthread_set_name_np()
 #endif
 

 #if defined(__linux__) && defined(SCHED_IDLE)
   // Some *really* old glibcs are missing SCHED_IDLE.
   // http://man7.org/linux/man-pages/man3/pthread_setschedparam.3.html
   // http://man7.org/linux/man-pages/man2/sched_setscheduler.2.html
   sched_param priority;
   // For each of the above policies, param->sched_priority must be 0.
   priority.sched_priority = 0;
   // SCHED_IDLE    for running very low priority background jobs.
   // SCHED_OTHER   the standard round-robin time-sharing policy;
   return !pthread_setschedparam(
              pthread_self(),
-             Priority == ThreadPriority::Background ? SCHED_IDLE : SCHED_OTHER,
+             // FIXME: consider SCHED_BATCH for Low
+             Priority == ThreadPriority::Default ? SCHED_OTHER : SCHED_IDLE,
              &priority)
              ? SetThreadPriorityResult::SUCCESS
              : SetThreadPriorityResult::FAILURE;
 #elif defined(__APPLE__)
-  // https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getpriority.2.html
-  // When setting a thread into background state the scheduling priority is set
-  // to lowest value, disk and network IO are throttled. Network IO will be
-  // throttled for any sockets the thread opens after going into background
-  // state. Any previously opened sockets are not affected.
-
-  // https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/getiopolicy_np.3.html
-  // I/Os with THROTTLE policy are called THROTTLE I/Os. If a THROTTLE I/O
-  // request occurs within a small time window (usually a fraction of a second)
-  // of another NORMAL I/O request, the thread that issues the THROTTLE I/O is
-  // forced to sleep for a certain interval. This slows down the thread that
-  // issues the THROTTLE I/O so that NORMAL I/Os can utilize most of the disk
-  // I/O bandwidth.
-  return !setpriority(PRIO_DARWIN_THREAD, 0,
-                      Priority == ThreadPriority::Background ? PRIO_DARWIN_BG
-                                                             : 0)
+  // https://developer.apple.com/documentation/apple-silicon/tuning-your-code-s-performance-for-apple-silicon
+  //
+  // Background - Applies to work that isn’t visible to the user and may take significant
+  // time to complete. Examples include indexing, backing up, or synchronizing data. This
+  // class emphasizes energy efficiency.
+  //
+  // Utility - Applies to work that takes anywhere from a few seconds to a few minutes to
+  // complete. Examples include downloading a document or importing data. This class
+  // offers a balance between responsiveness, performance, and energy efficiency.
+  const auto qosClass = [&](){
+    switch (Priority) {
+      case ThreadPriority::Background: return QOS_CLASS_BACKGROUND;
+      case ThreadPriority::Low: return QOS_CLASS_UTILITY;
+      case ThreadPriority::Default: return QOS_CLASS_DEFAULT;
+    }
+  }();
+  return !pthread_set_qos_class_self_np(qosClass, 0)
              ? SetThreadPriorityResult::SUCCESS
              : SetThreadPriorityResult::FAILURE;
 #endif
   return SetThreadPriorityResult::FAILURE;
 }
 
 #include <thread>
 

llvm/lib/Support/Windows/Threading.inc

 
 SetThreadPriorityResult llvm::set_thread_priority(ThreadPriority Priority) {
   // https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-setthreadpriority
   // Begin background processing mode. The system lowers the resource scheduling
   // priorities of the thread so that it can perform background work without
   // significantly affecting activity in the foreground.
   // End background processing mode. The system restores the resource scheduling
   // priorities of the thread as they were before the thread entered background
   // processing mode.
+  //
+  // FIXME: consider THREAD_PRIORITY_BELOW_NORMAL for Low
   return SetThreadPriority(GetCurrentThread(),
-                           Priority == ThreadPriority::Background
+                           Priority != ThreadPriority::Default
                                ? THREAD_MODE_BACKGROUND_BEGIN
                                : THREAD_MODE_BACKGROUND_END)
              ? SetThreadPriorityResult::SUCCESS
              : SetThreadPriorityResult::FAILURE;
 }
 
 struct ProcessorGroup {
   unsigned ID;