refactor ImportPriority

Summary:
This diff improves ImportPriority in a variety of ways.

It was originally intended that ImportPriority fit in a single 64-bit
integer. Replace the bitfield with a single uint64_t and manual
shifts. Also, add a static_assert to enforce the intent.

Use semantic constants to make it clear the relative import priorities
across EdenFS.

Add some unit tests.

Reviewed By: genevievehelsel

Differential Revision: D40602313

fbshipit-source-id: 3c99249ef2863396235ff1159b10b54d7478671d

eden/fs/benchmarks/hg_import_request_queue.cpp

@@ -42,7 +42,7 @@ void enqueue(benchmark::State& state) {
   std::vector<std::shared_ptr<HgImportRequest>> requests;
   requests.reserve(state.max_iterations);
   for (size_t i = 0; i < state.max_iterations; i++) {
-    requests.emplace_back(makeBlobImportRequest(ImportPriority::kNormal()));
+    requests.emplace_back(makeBlobImportRequest(kDefaultImportPriority));
   }
 
   auto requestIter = requests.begin();
@@ -60,7 +60,7 @@ void dequeue(benchmark::State& state) {
   auto queue = HgImportRequestQueue{edenConfig};
 
   for (size_t i = 0; i < state.max_iterations; i++) {
-    queue.enqueueBlob(makeBlobImportRequest(ImportPriority::kNormal()));
+    queue.enqueueBlob(makeBlobImportRequest(kDefaultImportPriority));
   }
 
   for (auto _ : state) {

eden/fs/inodes/RequestContext.h

@@ -64,7 +64,7 @@ class FsObjectFetchContext : public ObjectFetchContext {
   void deprioritize(uint64_t delta) override {
     ImportPriority prev = priority_.load(std::memory_order_acquire);
     priority_.compare_exchange_strong(
-        prev, prev.getDeprioritized(delta), std::memory_order_acq_rel);
+        prev, prev.adjusted(-delta), std::memory_order_acq_rel);
     if (getClientPid().has_value()) {
       XLOG(DBG7) << "priority for " << getClientPid().value()
                  << " has changed to: " << priority_.load().value();
@@ -81,8 +81,7 @@ class FsObjectFetchContext : public ObjectFetchContext {
    * might be rare cases where multiple threads access priority_
    * at the same time.
    */
-  std::atomic<ImportPriority> priority_{
-      ImportPriority(ImportPriorityKind::High)};
+  std::atomic<ImportPriority> priority_{kDefaultFsImportPriority};
 };
 
 using FsObjectFetchContextPtr = RefPtr<FsObjectFetchContext>;

eden/fs/inodes/TreePrefetchLease.h

@@ -31,7 +31,7 @@ class TreePrefetchLease {
         ObjectFetchContext::Cause cause)
         : clientPid_(clientPid), cause_(cause) {}
     ImportPriority getPriority() const override {
-      return ImportPriority::kLow();
+      return kReaddirPrefetchPriority;
     }
     std::optional<pid_t> getClientPid() const override {
       return clientPid_;

eden/fs/service/EdenServiceHandler.cpp

@@ -188,7 +188,7 @@ class PrefetchFetchContext : public ObjectFetchContext {
   }
 
   virtual ImportPriority getPriority() const override {
-    return ImportPriority::kLow();
+    return kThriftPrefetchPriority;
   }
 
   const std::unordered_map<std::string, std::string>* FOLLY_NULLABLE
@@ -1359,13 +1359,13 @@ void convertHgImportTraceEventToHgEvent(
   }
 
   switch (event.importPriority) {
-    case ImportPriorityKind::Low:
+    case ImportPriority::Class::Low:
       te.importPriority_ref() = HgImportPriority::LOW;
       break;
-    case ImportPriorityKind::Normal:
+    case ImportPriority::Class::Normal:
       te.importPriority_ref() = HgImportPriority::NORMAL;
       break;
-    case ImportPriorityKind::High:
+    case ImportPriority::Class::High:
       te.importPriority_ref() = HgImportPriority::HIGH;
       break;
   }

eden/fs/store/ImportPriority.cpp

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This software may be used and distributed according to the terms of the
+ * GNU General Public License version 2.
+ */
+
+#include "eden/fs/store/ImportPriority.h"
+
+namespace facebook::eden {
+
+std::string_view ImportPriority::className() const noexcept {
+  switch (getClass()) {
+    case Class::Low:
+      return "Low";
+    case Class::Normal:
+      return "Normal";
+    case Class::High:
+      return "High";
+  }
+  return "Unlabeled";
+}
+
+} // namespace facebook::eden

eden/fs/store/ImportPriority.h

@@ -7,64 +7,185 @@
 
 #pragma once
 
-#include <cstdint>
+#include <assert.h>
+#include <stdint.h>
+#include <string_view>
+#include <type_traits>
+
+#include <fmt/ostream.h>
 
 namespace facebook::eden {
 
-constexpr uint64_t kOffset = 60;
+/**
+ * 64-bit priority value. Effectively a pair of (ImportPriority::Class, offset),
+ * where offset is a signed integer used for dynamic priority adjustments.
+ *
+ * Dynamic priority adjustments do not change the priority class.
+ */
+class ImportPriority {
+ public:
+  enum class Class : uint8_t {
+    Low = 6,
+    Normal = 8,
+    High = 10,
+  };
 
-enum class ImportPriorityKind : uint8_t { Low = 0, Normal = 1, High = 2 };
+  explicit constexpr ImportPriority(
+      Class cls = Class::Normal,
+      int64_t adjustment = 0) noexcept
+      : value_{encode(cls, kDefaultOffset, adjustment)} {}
 
-struct ImportPriority {
-  ImportPriorityKind kind;
-  uint64_t offset : kOffset;
-
-  static constexpr ImportPriority kLow() {
-    return ImportPriority{ImportPriorityKind::Low};
-  }
-
-  static constexpr ImportPriority kNormal() {
-    return ImportPriority{ImportPriorityKind::Normal};
-  }
-
-  static constexpr ImportPriority kHigh() {
-    return ImportPriority{ImportPriorityKind::High};
-  }
-
-  // set half of the maximum offset as default offset to allow equal
-  // space for raising and lowering priority offset.
-  explicit constexpr ImportPriority() noexcept
-      : kind(ImportPriorityKind::Normal), offset(0x7FFFFFFFFFFF) {}
-  explicit constexpr ImportPriority(ImportPriorityKind kind) noexcept
-      : kind(kind), offset(0x7FFFFFFFFFFF) {}
-  constexpr ImportPriority(ImportPriorityKind kind, uint64_t offset) noexcept
-      : kind(kind), offset(offset) {}
-
-  constexpr inline uint64_t value() const noexcept {
-    return (static_cast<uint64_t>(kind) << kOffset) + offset;
+  static constexpr ImportPriority minimumValue() noexcept {
+    return ImportPriority{0};
   }
 
   /**
-   * Deprioritize ImportPriority by decreasing offset by delta.
-   * Note: this function maintains ImportPriorityKind, as jobs
-   * with higher priority kind are usually designed to be scheduled
-   * ealier and should not lower their kind even when deprioritized.
+   * Returns the priority class component of the priority value.
    */
-  constexpr ImportPriority getDeprioritized(uint64_t delta) const noexcept {
-    return ImportPriority{kind, offset > delta ? offset - delta : 0};
+  constexpr Class getClass() const noexcept {
+    return static_cast<Class>(value_ >> kClassShift);
   }
 
-  friend bool operator<(
-      const ImportPriority& lhs,
-      const ImportPriority& rhs) noexcept {
-    return lhs.value() < rhs.value();
+  /**
+   * Returns the adjustment component of the priority value.
+   */
+  constexpr int64_t getAdjustment() const noexcept {
+    uint64_t offset = value_ & kOffsetMask;
+    return static_cast<int64_t>(offset) - kDefaultOffset;
   }
 
-  friend bool operator>(
-      const ImportPriority& lhs,
-      const ImportPriority& rhs) noexcept {
-    return lhs.value() > rhs.value();
+  /**
+   * Returns a human-readable priority class name.
+   */
+  std::string_view className() const noexcept;
+
+  /**
+   * Returns an opaque uint64_t whose only guarantee is that it can be sorted
+   * the same way an ImportPriority can.
+   */
+  constexpr uint64_t value() const noexcept {
+    return value_;
   }
+
+  /**
+   * Returns a new ImportPriority at a given offset from this. Positive values
+   * increase priority; negatives decrease.
+   *
+   * The priority class will not change. This is intentional, as jobs with high
+   * priority class are usually designed to be scheduled earlier even under
+   * dynamic prioritization. However, it's somewhat academic, as 60 bits is
+   * overkill.
+   */
+  constexpr ImportPriority adjusted(int64_t delta) const noexcept {
+    Class k = getClass();
+    uint64_t offset = value_ & kOffsetMask;
+    return ImportPriority{encode(k, offset, delta)};
+  }
+
+  friend bool operator==(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ == rhs.value_;
+  }
+
+  friend bool operator!=(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ != rhs.value_;
+  }
+
+  friend bool operator<(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ < rhs.value_;
+  }
+
+  friend bool operator<=(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ <= rhs.value_;
+  }
+
+  friend bool operator>(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ > rhs.value_;
+  }
+
+  friend bool operator>=(ImportPriority lhs, ImportPriority rhs) noexcept {
+    return lhs.value_ >= rhs.value_;
+  }
+
+ private:
+  explicit constexpr ImportPriority(uint64_t raw) noexcept : value_{raw} {}
+
+  static constexpr uint64_t
+  encode(Class cls, uint64_t initialOffset, int64_t adjustment) noexcept {
+    uint64_t k = static_cast<std::underlying_type_t<Class>>(cls);
+    assert(k < 16 && "Priority class must fit in a nibble");
+
+    assert(
+        0 == (initialOffset >> kClassShift) &&
+        "Initial offset must not overflow into class bits");
+
+    // There has to be a better way of writing the following clamp operation.
+    // Another approach would be to ignore clamping entirely, because 60 bits is
+    // plenty.
+    uint64_t offset = initialOffset;
+    if (adjustment > 0) {
+      uint64_t positiveOffset = static_cast<uint64_t>(adjustment);
+      if (offset > kMaximumOffset - positiveOffset) {
+        offset = kMaximumOffset;
+      } else {
+        offset += positiveOffset;
+      }
+    } else if (adjustment < 0) {
+      uint64_t negativeOffset = static_cast<uint64_t>(-adjustment);
+      if (offset < negativeOffset) {
+        offset = 0;
+      } else {
+        offset -= negativeOffset;
+      }
+    }
+
+    assert(
+        0 == (offset >> kClassShift) &&
+        "Adjusted offset must not overflow into class bits");
+    return (k << kClassShift) | offset;
+  }
+
+  // Class is stored in the high nibble. Set the default offset to the midpoint
+  // of a 60-bit integer so we can increase and decrease priority without
+  // overflowing into the class bits.
+  static inline constexpr uint64_t kDefaultOffset = 0x0800'0000'0000'0000ull;
+  static inline constexpr uint64_t kMinimumOffset = 0ull;
+  static inline constexpr uint64_t kMaximumOffset = 0x0FFF'FFFF'FFFF'FFFFull;
+  static inline constexpr uint64_t kClassShift = 60ull;
+  static inline constexpr uint64_t kOffsetMask = (1ull << kClassShift) - 1ull;
+
+  uint64_t value_;
 };
 
+// Centralized list of default priorities so their relative order is clear.
+
+inline constexpr ImportPriority kDefaultImportPriority{
+    ImportPriority::Class::Normal};
+inline constexpr ImportPriority kDefaultFsImportPriority{
+    ImportPriority::Class::High};
+inline constexpr ImportPriority kReaddirPrefetchPriority{
+    ImportPriority::Class::Low};
+inline constexpr ImportPriority kThriftPrefetchPriority{
+    ImportPriority::Class::Low};
+
 } // namespace facebook::eden
+
+namespace std {
+inline std::ostream& operator<<(
+    std::ostream& os,
+    facebook::eden::ImportPriority priority) {
+  return os << "(" << priority.className() << ", " << priority.getAdjustment()
+            << ")";
+}
+} // namespace std
+
+template <>
+struct fmt::formatter<facebook::eden::ImportPriority> {
+  template <typename Context>
+  auto format(facebook::eden::ImportPriority priority, Context& ctx) const {
+    return format_to(
+        ctx.out(),
+        "({}, {:+d})",
+        priority.className(),
+        priority.getAdjustment());
+  }
+};

eden/fs/store/ObjectFetchContext.h

@@ -84,7 +84,7 @@ class ObjectFetchContext : public RefCounted {
   }
 
   virtual ImportPriority getPriority() const {
-    return ImportPriority::kNormal();
+    return kDefaultImportPriority;
   }
 
   virtual const std::unordered_map<std::string, std::string>* getRequestInfo()

eden/fs/store/hg/HgImportRequestQueue.cpp

@@ -124,7 +124,7 @@ std::vector<std::shared_ptr<HgImportRequest>> HgImportRequestQueue::dequeue() {
       return std::vector<std::shared_ptr<HgImportRequest>>();
     }
 
-    ImportPriority highestPriority{ImportPriorityKind::Low, 0};
+    auto highestPriority = ImportPriority::minimumValue();
 
     // Trees have a higher priority than blobs, thus check the queues in that
     // order.  The reason for trees having a higher priority is due to trees

eden/fs/store/hg/HgQueuedBackingStore.cpp

@@ -15,6 +15,7 @@
 #include <re2/re2.h>
 
 #include <folly/Range.h>
+#include <folly/String.h>
 #include <folly/futures/Future.h>
 #include <folly/logging/xlog.h>
 #include <folly/portability/GFlags.h>
@@ -38,8 +39,6 @@
 #include "eden/fs/utils/PathFuncs.h"
 #include "eden/fs/utils/StaticAssert.h"
 #include "eden/fs/utils/Throw.h"
-#include "folly/ScopeGuard.h"
-#include "folly/String.h"
 
 namespace facebook::eden {
 
@@ -58,7 +57,7 @@ HgImportTraceEvent::HgImportTraceEvent(
     EventType eventType,
     ResourceType resourceType,
     const HgProxyHash& proxyHash,
-    ImportPriorityKind priority,
+    ImportPriority::Class priority,
     ObjectFetchContext::Cause cause)
     : unique{unique},
       manifestNodeId{proxyHash.revHash()},
@@ -148,7 +147,7 @@ void HgQueuedBackingStore::processBlobImportRequests(
         request->getUnique(),
         HgImportTraceEvent::BLOB,
         blobImport->proxyHash,
-        request->getPriority().kind,
+        request->getPriority().getClass(),
         request->getCause()));
 
     XLOGF(DBG4, "Processing blob request for {}", blobImport->hash);
@@ -201,7 +200,7 @@ void HgQueuedBackingStore::processTreeImportRequests(
         request->getUnique(),
         HgImportTraceEvent::TREE,
         treeImport->proxyHash,
-        request->getPriority().kind,
+        request->getPriority().getClass(),
         request->getCause()));
 
     XLOGF(DBG4, "Processing tree request for {}", treeImport->hash);
@@ -420,7 +419,7 @@ HgQueuedBackingStore::getTreeImpl(
         unique,
         HgImportTraceEvent::TREE,
         proxyHash,
-        context->getPriority().kind,
+        context->getPriority().getClass(),
         context->getCause()));
 
     return queue_.enqueueTree(std::move(request))
@@ -433,7 +432,7 @@ HgQueuedBackingStore::getTreeImpl(
               unique,
               HgImportTraceEvent::TREE,
               proxyHash,
-              context->getPriority().kind,
+              context->getPriority().getClass(),
               context->getCause()));
         });
   });
@@ -491,7 +490,7 @@ HgQueuedBackingStore::getBlobImpl(
         unique,
         HgImportTraceEvent::BLOB,
         proxyHash,
-        context->getPriority().kind,
+        context->getPriority().getClass(),
         context->getCause()));
 
     return queue_.enqueueBlob(std::move(request))
@@ -504,7 +503,7 @@ HgQueuedBackingStore::getBlobImpl(
               unique,
               HgImportTraceEvent::BLOB,
               proxyHash,
-              context->getPriority().kind,
+              context->getPriority().getClass(),
               context->getCause()));
         });
   });

eden/fs/store/hg/HgQueuedBackingStore.h

@@ -50,7 +50,7 @@ struct HgImportTraceEvent : TraceEventBase {
       uint64_t unique,
       ResourceType resourceType,
       const HgProxyHash& proxyHash,
-      ImportPriorityKind priority,
+      ImportPriority::Class priority,
       ObjectFetchContext::Cause cause) {
     return HgImportTraceEvent{
         unique, QUEUE, resourceType, proxyHash, priority, cause};
@@ -60,7 +60,7 @@ struct HgImportTraceEvent : TraceEventBase {
       uint64_t unique,
       ResourceType resourceType,
       const HgProxyHash& proxyHash,
-      ImportPriorityKind priority,
+      ImportPriority::Class priority,
       ObjectFetchContext::Cause cause) {
     return HgImportTraceEvent{
         unique, START, resourceType, proxyHash, priority, cause};
@@ -70,7 +70,7 @@ struct HgImportTraceEvent : TraceEventBase {
       uint64_t unique,
       ResourceType resourceType,
       const HgProxyHash& proxyHash,
-      ImportPriorityKind priority,
+      ImportPriority::Class priority,
       ObjectFetchContext::Cause cause) {
     return HgImportTraceEvent{
         unique, FINISH, resourceType, proxyHash, priority, cause};
@@ -81,7 +81,7 @@ struct HgImportTraceEvent : TraceEventBase {
       EventType eventType,
       ResourceType resourceType,
       const HgProxyHash& proxyHash,
-      ImportPriorityKind priority,
+      ImportPriority::Class priority,
       ObjectFetchContext::Cause cause);
 
   // Simple accessor that hides the internal memory representation of paths.
@@ -98,7 +98,7 @@ struct HgImportTraceEvent : TraceEventBase {
   Hash20 manifestNodeId;
   EventType eventType;
   ResourceType resourceType;
-  ImportPriorityKind importPriority;
+  ImportPriority::Class importPriority;
   ObjectFetchContext::Cause importCause;
 };
 

eden/fs/store/hg/test/HgImportRequestQueueTest.cpp

@@ -111,14 +111,14 @@ TEST_F(HgImportRequestQueueTest, getRequestByPriority) {
 
   for (int i = 0; i < 10; i++) {
     auto [hash, request] =
-        makeBlobImportRequest(ImportPriority(ImportPriorityKind::Normal, i));
+        makeBlobImportRequest(ImportPriority(ImportPriority::Class::Normal, i));
 
     queue.enqueueBlob(std::move(request));
     enqueued.push_back(hash);
   }
 
   auto [smallHash, smallRequest] =
-      makeBlobImportRequest(ImportPriority(ImportPriorityKind::Low, 0));
+      makeBlobImportRequest(ImportPriority(ImportPriority::Class::Low, 0));
 
   queue.enqueueBlob(std::move(smallRequest));
 
@@ -159,14 +159,14 @@ TEST_F(HgImportRequestQueueTest, getRequestByPriorityReverse) {
 
   for (int i = 0; i < 10; i++) {
     auto [hash, request] = makeBlobImportRequest(
-        ImportPriority(ImportPriorityKind::Normal, 10 - i));
+        ImportPriority(ImportPriority::Class::Normal, 10 - i));
 
     queue.enqueueBlob(std::move(request));
     enqueued.push_back(hash);
   }
 
   auto [largeHash, largeRequest] =
-      makeBlobImportRequest(ImportPriority(ImportPriority::kHigh()));
+      makeBlobImportRequest(ImportPriority{ImportPriority::Class::High});
 
   queue.enqueueBlob(std::move(largeRequest));
 
@@ -208,11 +208,11 @@ TEST_F(HgImportRequestQueueTest, mixedPriority) {
   for (int i = 0; i < 10; i++) {
     {
       auto hash = insertBlobImportRequest(
-          queue, ImportPriority(ImportPriorityKind::Normal, i));
+          queue, ImportPriority(ImportPriority::Class::Normal, i));
       enqueued_blob.emplace(hash);
     }
     auto hash = insertTreeImportRequest(
-        queue, ImportPriority(ImportPriorityKind::Normal, 10 - i));
+        queue, ImportPriority(ImportPriority::Class::Normal, 10 - i));
     enqueued_tree.emplace(hash);
   }
 
@@ -232,7 +232,7 @@ TEST_F(HgImportRequestQueueTest, mixedPriority) {
         enqueued_tree.end());
     EXPECT_TRUE(
         dequeuedRequest->getPriority().value() ==
-        ImportPriority(ImportPriorityKind::Normal, 10 - i)
+        ImportPriority(ImportPriority::Class::Normal, 10 - i)
             .value()); // assert tree requests of priority 10 and 9
 
     folly::Try<std::unique_ptr<Tree>> tree = folly::makeTryWith(
@@ -257,7 +257,7 @@ TEST_F(HgImportRequestQueueTest, mixedPriority) {
         enqueued_blob.end());
     EXPECT_TRUE(
         dequeuedRequest->getPriority().value() ==
-        ImportPriority(ImportPriorityKind::Normal, 9 - i)
+        ImportPriority(ImportPriority::Class::Normal, 9 - i)
             .value()); // assert blob requests of priority 9, 8, and 7
 
     folly::Try<std::unique_ptr<Blob>> blob = folly::makeTryWith(
@@ -278,11 +278,11 @@ TEST_F(HgImportRequestQueueTest, getMultipleRequests) {
   for (int i = 0; i < 10; i++) {
     {
       auto hash = insertBlobImportRequest(
-          queue, ImportPriority(ImportPriorityKind::Normal, 0));
+          queue, ImportPriority{ImportPriority::Class::Normal});
       enqueued_blob.emplace(hash);
     }
     auto hash = insertTreeImportRequest(
-        queue, ImportPriority(ImportPriorityKind::Normal, 0));
+        queue, ImportPriority{ImportPriority::Class::Normal});
     enqueued_tree.emplace(hash);
   }
 
@@ -337,10 +337,10 @@ TEST_F(HgImportRequestQueueTest, duplicateRequestAfterEnqueue) {
   auto proxyHash = HgProxyHash{RelativePath{"some_blob"}, hgRevHash};
 
   auto [hash, request] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash2, request2] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   queue.enqueueBlob(std::move(request));
   enqueued.push_back(hash);
@@ -373,10 +373,10 @@ TEST_F(HgImportRequestQueueTest, duplicateRequestAfterDequeue) {
   auto proxyHash = HgProxyHash{RelativePath{"some_blob"}, hgRevHash};
 
   auto [hash, request] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash2, request2] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   queue.enqueueBlob(std::move(request));
   enqueued.push_back(hash);
@@ -411,10 +411,10 @@ TEST_F(HgImportRequestQueueTest, duplicateRequestAfterMarkedDone) {
   auto proxyHash = HgProxyHash{RelativePath{"some_blob"}, hgRevHash};
 
   auto [hash, request] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash2, request2] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   queue.enqueueBlob(std::move(request));
   enqueued.push_back(hash);
@@ -446,16 +446,16 @@ TEST_F(HgImportRequestQueueTest, multipleDuplicateRequests) {
   auto proxyHash = HgProxyHash{RelativePath{"some_blob"}, hgRevHash};
 
   auto [hash, request] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash2, request2] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash3, request3] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   auto [hash4, request4] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 5), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 5), proxyHash);
 
   queue.enqueueBlob(std::move(request2));
   queue.enqueueBlob(std::move(request));
@@ -492,14 +492,14 @@ TEST_F(HgImportRequestQueueTest, twoDuplicateRequestsDifferentPriority) {
   auto proxyHash = HgProxyHash{RelativePath{"some_blob"}, hgRevHash};
 
   auto [midPriHash, midPriRequest] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 6), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 6), proxyHash);
 
   auto [lowPriHash, lowPriRequest] = makeBlobImportRequestWithHash(
-      ImportPriority(ImportPriorityKind::Normal, 0), proxyHash);
+      ImportPriority(ImportPriority::Class::Normal, 0), proxyHash);
 
   for (int i = 1; i < 6; i++) {
     auto [hash, request] =
-        makeBlobImportRequest(ImportPriority(ImportPriorityKind::Normal, i));
+        makeBlobImportRequest(ImportPriority(ImportPriority::Class::Normal, i));
 
     queue.enqueueBlob(std::move(request));
     enqueued.push_back(hash);
@@ -507,7 +507,7 @@ TEST_F(HgImportRequestQueueTest, twoDuplicateRequestsDifferentPriority) {
 
   for (int i = 7; i < 11; i++) {
     auto [hash, request] =
-        makeBlobImportRequest(ImportPriority(ImportPriorityKind::Normal, i));
+        makeBlobImportRequest(ImportPriority(ImportPriority::Class::Normal, i));
 
     queue.enqueueBlob(std::move(request));
     enqueued.push_back(hash);

eden/fs/store/test/ImportPriorityTest.cpp

@@ -11,17 +11,45 @@
 #include <folly/portability/GMock.h>
 #include <folly/portability/GTest.h>
 
+#include "eden/fs/utils/StaticAssert.h"
+
+namespace {
+
 using namespace facebook::eden;
 
-TEST(ImportPriorityTest, value) {
-  EXPECT_LT(ImportPriority::kNormal(), ImportPriority::kHigh());
-  EXPECT_LT(ImportPriority::kLow(), ImportPriority::kNormal());
+static_assert(CheckSize<ImportPriority, sizeof(uint64_t)>());
 
-  // The maximum possible priority
-  auto maximum = ImportPriority{ImportPriorityKind::High, 0xFFFFFFFFFFFFFFF};
-  EXPECT_EQ(maximum.value(), 0x2FFFFFFFFFFFFFFF);
-
-  // the minimum possible priority
-  auto minimum = ImportPriority{ImportPriorityKind::Low, 0};
-  EXPECT_EQ(minimum.value(), 0x0000000000000000);
+TEST(ImportPriorityTest, basic_class_comparison) {
+  EXPECT_LT(
+      ImportPriority{ImportPriority::Class::Normal},
+      ImportPriority{ImportPriority::Class::High});
+  EXPECT_LT(
+      ImportPriority{ImportPriority::Class::Low},
+      ImportPriority{ImportPriority::Class::Normal});
 }
+
+TEST(ImportPriorityTest, deprioritized_keeps_class_but_compares_lower) {
+  auto initial = ImportPriority{};
+  auto lower = initial.adjusted(-1);
+  EXPECT_EQ(initial.getClass(), lower.getClass());
+  EXPECT_LT(lower, initial);
+}
+
+TEST(ImportPriorityTest, format) {
+  EXPECT_EQ(
+      "(Normal, +0)",
+      fmt::to_string(ImportPriority{ImportPriority::Class::Normal}));
+  EXPECT_EQ(
+      "(High, -10)",
+      fmt::to_string(ImportPriority{ImportPriority::Class::High, -10}));
+  EXPECT_EQ(
+      "(Low, +10)",
+      fmt::to_string(ImportPriority{ImportPriority::Class::Low, 10}));
+}
+
+TEST(ImportPriorityTest, minimum_value_cannot_be_deprioritized) {
+  auto minimum = ImportPriority::minimumValue();
+  EXPECT_EQ(minimum, minimum.adjusted(-1));
+}
+
+} // namespace