Back out D88004735-D88260777

Summary:
There are multiple issues with that stack:

* glean CI is broken (and we don't now how to fix it) 3867100#commitcomment-172635196
* It degraded the time of fillOwnership processing
* We really need to test `configerator` db first, because we use chronos write server there and the complete step already takes 37 GB. (see the spike on memory usage https://www.internalfb.com/chronos/job_instance/gp/81064803470734278/info). And having u64 sets will only increase it.
* I face a crash trying to connect to the db which was produced with this change: `test.1/1: couldn't open: corrupt database - invalid FIRST_UNIT_ID`
* There are also couple of comments from Simon Marlow about not using justknob in `database` target and using a db version for controlling the format.

So, given all that we have to revert it before it publishes on Monday. After that I think we don't need to move to U64 format at all, just change TrieArray implementation with proper profiling.

Reviewed By: jjuliamolin, phlalx

Differential Revision: D89051067

fbshipit-source-id: 0c6858ab1ffd42b8d2d6d33a5d2e795d3b40bf41

Author: iamirzhan

glean/db/Glean/Database/Ownership.hs

@@ -16,6 +16,7 @@ import Data.ByteString (ByteString)
 import Data.Coerce
 import Data.Maybe
 import qualified Data.Vector.Storable as Vector
+import Data.Word
 
 import Util.STM
 
@@ -55,7 +56,7 @@ factOwnership env repo fid = do
         Nothing -> getUnit unitId rest
 
     getUset usetId [] = throwIO $ ErrorCall $
-      "unknown UsetId: " <> show usetId
+      "unknown UsetId: " <> show (coerce usetId :: Word32)
     getUset usetId (own : owns) = do
       maybeExpr <- getOwnershipSet own usetId
       case maybeExpr of

glean/hs/Glean/RTS/Foreign/Ownership.hsc

@@ -79,12 +79,12 @@ instance Static UnitIterator where
   destroyStatic = glean_ownership_unit_iterator_free
 
 -- | Id of a unit
-newtype UnitId = UnitId Word64
+newtype UnitId = UnitId Word32
   deriving (Storable, Show)
 
 -- | Id of an ownership set
-newtype UsetId = UsetId Word64
-  deriving (Storable, Show)
+newtype UsetId = UsetId Word32
+  deriving (Storable)
 
 firstUsetId :: UsetId
 firstUsetId = UsetId 0
@@ -288,7 +288,7 @@ getOwnershipSet ownership usetid =
           let op | cop == (#const facebook::glean::rts::Or) = Or
                  | cop == (#const facebook::glean::rts::And) = And
                  | otherwise = error "unknown SetOp"
-          return $ Just (op, unsafeCoerceVector (vec :: VS.Vector Word64))
+          return $ Just (op, unsafeCoerceVector (vec :: VS.Vector Word32))
     )
 
 data OwnershipStats = OwnershipStats
@@ -420,14 +420,14 @@ foreign import ccall safe glean_ownership_compute
 foreign import ccall unsafe glean_get_fact_owner
   :: Ptr Lookup
   -> Word64
-  -> Ptr Word64
+  -> Ptr Word32
   -> IO CString
 
 foreign import ccall unsafe glean_get_ownership_set
   :: Ptr Ownership
   -> UsetId
   -> Ptr CInt
-  -> Ptr (Ptr (HsArray Word64))
+  -> Ptr (Ptr (HsArray Word32))
   -> IO CString
 
 foreign import ccall safe glean_derived_ownership_compute
@@ -445,7 +445,7 @@ foreign import ccall unsafe "&glean_computed_ownership_free"
 
 foreign import ccall safe glean_slice_compute
   :: Ptr Ownership
-  -> Ptr Word64
+  -> Ptr Word32
   -> CSize
   -> CInt
   -> Ptr (Ptr Slice)

glean/rocksdb/database-impl.cpp

@@ -11,10 +11,6 @@
 
 #include "glean/rts/timer.h"
 
-#ifndef OSS
-#include "justknobs/JustKnobProxy.h"
-#endif
-
 namespace facebook {
 namespace glean {
 namespace rocks {
@@ -29,49 +25,18 @@ const char* admin_names[] = {
     "FIRST_UNIT_ID",
     "NEXT_UNIT_ID",
     "ORPHAN_FACTS",
-    "OWNERSHIP_FORMAT_VERSION",
 };
 
 namespace {
 
-// Read an admin value with backward compatibility for 32-bit -> 64-bit
-// migration. If the stored value is 4 bytes (32-bit) and we expect 8 bytes
-// (64-bit), read the 32-bit value and widen it.
-template <typename T>
-folly::Optional<T> readAdminValueCompat(ContainerImpl& container_, AdminId id) {
-  rocksdb::PinnableSlice val;
-  auto s = container_.db->Get(
-      rocksdb::ReadOptions(),
-      container_.family(Family::admin),
-      toSlice(id),
-      &val);
-  if (s.IsNotFound()) {
-    return folly::none;
-  }
-  check(s);
-
-  // Handle backward compatibility: if stored as 32-bit but expected as 64-bit
-  if constexpr (sizeof(T) == 8) {
-    if (val.size() == 4) {
-      // Old 32-bit value, widen to 64-bit
-      binary::Input inp(byteRange(val));
-      return static_cast<T>(inp.fixed<uint32_t>());
-    }
-  }
-
-  // Normal case: size matches expected
-  binary::Input inp(byteRange(val));
-  return inp.fixed<T>();
-}
-
 template <typename T, typename F>
 T initAdminValue(
     ContainerImpl& container_,
     AdminId id,
     T def,
     bool write,
     F&& notFound) {
-  auto current = readAdminValueCompat<T>(container_, id);
+  auto current = readAdminValue<T>(container_, id);
   if (current.hasValue()) {
     return *current;
   } else {
@@ -150,44 +115,6 @@ DatabaseImpl::DatabaseImpl(
     rts::error("unexpected database version {}", db_version);
   }
 
-  // Initialize ownership format version
-  // For new DBs: Check JustKnob to decide whether to use 64-bit or 32-bit
-  // format. For existing DBs: if no version marker exists, assume 32-bit format
-  // Both formats are always readable regardless of gatekeeper value
-  if (container_.mode == Mode::Create) {
-    // New DB: check JustKnob to determine format
-#ifndef OSS
-    static facebook::jk::BooleanKnob use64BitOwnership(
-        "glean/ownership:64_bit_ids");
-    uint32_t format_to_use = use64BitOwnership()
-        ? OWNERSHIP_FORMAT_VERSION_64BIT
-        : OWNERSHIP_FORMAT_VERSION_32BIT;
-#else
-    // OSS builds always use 32-bit format for now
-    uint32_t format_to_use = OWNERSHIP_FORMAT_VERSION_32BIT;
-#endif
-    ownership_format_version = static_cast<uint32_t>(initAdminValue(
-        container_,
-        AdminId::OWNERSHIP_FORMAT_VERSION,
-        static_cast<uint64_t>(format_to_use),
-        true, // write
-        [] {}));
-  } else {
-    // Existing DB: read format version, default to 32-bit if not present
-    ownership_format_version = static_cast<uint32_t>(initAdminValue(
-        container_,
-        AdminId::OWNERSHIP_FORMAT_VERSION,
-        static_cast<uint64_t>(OWNERSHIP_FORMAT_VERSION_32BIT),
-        false, // don't write
-        [] {}));
-  }
-
-  VLOG(1) << folly::sformat(
-      "ownership_format_version: {} ({})",
-      ownership_format_version,
-      ownership_format_version == OWNERSHIP_FORMAT_VERSION_32BIT ? "32-bit"
-                                                                 : "64-bit");
-
   stats_.set(loadStats());
 
   if (container_.mode != Mode::ReadOnly) {

glean/rocksdb/database-impl.h

@@ -29,17 +29,8 @@ enum class AdminId : uint32_t {
   FIRST_UNIT_ID,
   NEXT_UNIT_ID,
   ORPHAN_FACTS,
-  OWNERSHIP_FORMAT_VERSION,
 };
 
-// Ownership format versions for backward compatibility:
-// - Version 1 (32-bit/absent): 32-bit UnitId and UsetId
-// - Version 2: 64-bit UnitId and UsetId
-constexpr uint32_t OWNERSHIP_FORMAT_VERSION_32BIT = 1;
-constexpr uint32_t OWNERSHIP_FORMAT_VERSION_64BIT = 2;
-constexpr uint32_t OWNERSHIP_FORMAT_VERSION_CURRENT =
-    OWNERSHIP_FORMAT_VERSION_64BIT;
-
 struct DatabaseImpl final : Database {
   int64_t db_version;
   ContainerImpl container_;
@@ -51,11 +42,6 @@ struct DatabaseImpl final : Database {
   std::vector<size_t> ownership_unit_counters;
   folly::F14FastMap<uint64_t, size_t> ownership_derived_counters;
 
-  // Ownership format version for backward compatibility
-  // OWNERSHIP_FORMAT_VERSION_32BIT (1): 32-bit UnitId/UsetId (or absent =
-  // 32-bit) OWNERSHIP_FORMAT_VERSION_64BIT (2): 64-bit UnitId/UsetId
-  uint32_t ownership_format_version;
-
   // Cached ownership sets, only used when writing.
   // Note: must only be accessed under the write lock
   std::unique_ptr<rts::Usets> usets_;
@@ -124,8 +110,8 @@ struct DatabaseImpl final : Database {
 
   /// Ownership
 
-  folly::Optional<rts::UnitId> getUnitId(folly::ByteRange unit) override;
-  folly::Optional<std::string> getUnit(rts::UnitId unit_id) override;
+  folly::Optional<uint32_t> getUnitId(folly::ByteRange unit) override;
+  folly::Optional<std::string> getUnit(uint32_t unit_id) override;
 
   void addOwnership(const std::vector<OwnershipSet>& ownership) override;
 
@@ -191,7 +177,7 @@ struct DatabaseImpl final : Database {
 
   struct FactOwnerCache {
     static void prepare(ContainerImpl& container);
-    void enable(ContainerImpl& container, uint32_t ownership_format_version);
+    void enable(ContainerImpl& container);
 
     // Lookup in the cache. Returns none if the cache is not enabled
     std::optional<rts::UsetId> getOwner(ContainerImpl& container, Id id);
@@ -204,18 +190,14 @@ struct DatabaseImpl final : Database {
     static rts::UsetId lookup(const Page& page, Id id);
     static std::unique_ptr<Page> readPage(
         ContainerImpl& container,
-        uint64_t prefix,
-        uint32_t ownership_format_version);
+        uint64_t prefix);
 
     struct Cache {
       std::vector<rts::UsetId> index;
       std::vector<std::unique_ptr<Page>> pages;
 
       // tracks the memory usage of the cache
       size_t size_;
-
-      // ownership format version for legacy compatibility
-      uint32_t ownership_format_version_;
     };
 
     // nullptr means the cache is disabled (while the DB is writable)