From 984822867517f42f8f8496c3c9ecf2fe7ef44c05 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Mon, 23 Oct 2023 10:26:13 +0200
Subject: [PATCH 1/7] Vendor hasUniqueRepresentation to account for packed
 structs

Works around https://github.com/ziglang/zig/issues/17592.
---
 src/stdx.zig | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/stdx.zig b/src/stdx.zig
index c9b7c7b3c6..7cee678147 100644
--- a/src/stdx.zig
+++ b/src/stdx.zig
@@ -242,7 +242,7 @@ else
 ///   - `T` doesn't have any non-deterministic padding,
 ///   - `T` doesn't embed any pointers.
 pub fn equal_bytes(comptime T: type, a: *const T, b: *const T) bool {
-    comptime assert(std.meta.trait.hasUniqueRepresentation(T));
+    comptime assert(hasUniqueRepresentation(T));
     comptime assert(!has_pointers(T));
     comptime assert(@sizeOf(T) * 8 == @bitSizeOf(T));
 
@@ -339,7 +339,7 @@ test no_padding {
 pub inline fn hash_inline(value: anytype) u64 {
     comptime {
         assert(no_padding(@TypeOf(value)));
-        assert(std.meta.trait.hasUniqueRepresentation(@TypeOf(value)));
+        assert(hasUniqueRepresentation(@TypeOf(value)));
     }
     return low_level_hash(0, switch (@typeInfo(@TypeOf(value))) {
         .Struct, .Int => std.mem.asBytes(&value),
@@ -520,3 +520,46 @@ pub fn fstatfs(fd: i32, statfs_buf: *StatFs) usize {
         @intFromPtr(statfs_buf),
     );
 }
+
+// TODO(Zig): https://github.com/ziglang/zig/issues/17592.
+/// True if every value of the type `T` has a unique bit pattern representing it.
+/// In other words, `T` has no unused bits and no padding.
+pub fn hasUniqueRepresentation(comptime T: type) bool {
+    switch (@typeInfo(T)) {
+        else => return false, // TODO can we know if it's true for some of these types ?
+
+        .AnyFrame,
+        .Enum,
+        .ErrorSet,
+        .Fn,
+        => return true,
+
+        .Bool => return false,
+
+        .Int => |info| return @sizeOf(T) * 8 == info.bits,
+
+        .Pointer => |info| return info.size != .Slice,
+
+        .Array => |info| return comptime hasUniqueRepresentation(info.child),
+
+        .Struct => |info| {
+            // Packed structs are always unique.
+            if (info.backing_integer != null) {
+                return true;
+            }
+
+            var sum_size = @as(usize, 0);
+
+            inline for (info.fields) |field| {
+                const FieldType = field.type;
+                if (comptime !hasUniqueRepresentation(FieldType)) return false;
+                sum_size += @sizeOf(FieldType);
+            }
+
+            return @sizeOf(T) == sum_size;
+        },
+
+        .Vector => |info| return comptime hasUniqueRepresentation(info.child) and
+            @sizeOf(T) == @sizeOf(info.child) * info.len,
+    }
+}

From 66fab94bf74dc7968702245e962f89bb3da4a3c1 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Mon, 23 Oct 2023 10:27:58 +0200
Subject: [PATCH 2/7] LSM: Revert groove to use update + insert Save a lookup
 by requiring the old object to be passed in to the Groove, and only check if
 constants.verify is true.

---
 src/lsm/groove.zig            | 38 +++++++++++++++++++++--------------
 src/state_machine.zig         | 20 +++++++++---------
 src/testing/state_machine.zig |  2 +-
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/src/lsm/groove.zig b/src/lsm/groove.zig
index 959c20fb3d..37c22e72df 100644
--- a/src/lsm/groove.zig
+++ b/src/lsm/groove.zig
@@ -863,10 +863,12 @@ pub fn GrooveType(
             }
         };
 
-        /// Insert the value into the objects tree and associated index trees, asserting that it doesn't
-        /// already exist.
+        /// Insert the value into the objects tree and associated index trees. It's up to the
+        /// caller to ensure it doesn't already exist.
         pub fn insert(groove: *Groove, object: *const Object) void {
-            assert(!groove.objects_cache.has(@field(object, primary_field)));
+            if (constants.verify) {
+                assert(!groove.objects_cache.has(@field(object, primary_field)));
+            }
 
             groove.objects_cache.upsert(object);
 
@@ -886,37 +888,43 @@ pub fn GrooveType(
             }
         }
 
-        /// Insert the value (or update it, if it exists).
+        /// Update the value. Requires the old object to be provided.
         /// Update the object and index trees by diff'ing the old and new values.
-        pub fn upsert(groove: *Groove, new: *const Object) void {
-            // Explicit stack copy needed - otherwise old will == new!
-            const old: Object = (groove.objects_cache.get(@field(new, primary_field)) orelse {
-                return groove.insert(new);
-            }).*;
+        pub fn update(groove: *Groove, new: *const Object, old: *const Object) void {
+            if (constants.verify) {
+                const old_from_cache = groove.objects_cache.get(@field(old, primary_field)).?;
+                assert(@field(old_from_cache, primary_field) == @field(old, primary_field));
+                assert(old_from_cache.timestamp == old.timestamp);
+            }
+
+            // Sanity check to ensure the caller didn't accidentally pass in an alias.
+            assert(new != old);
 
+            // TODO: This will assert timestamp twice if `has_id` is false.
             assert(@field(old, primary_field) == @field(new, primary_field));
             assert(old.timestamp == new.timestamp);
 
-            groove.objects_cache.upsert(new);
-
             // The ID can't change, so no need to update the ID tree.
-
             // Update the object tree entry if any of the fields (even ignored) are different.
-            if (!std.mem.eql(u8, std.mem.asBytes(&old), std.mem.asBytes(new))) {
+            if (!stdx.equal_bytes(Object, old, new)) {
                 // Unlike the index trees, the new and old values in the object tree share the
                 // same key. Therefore put() is sufficient to overwrite the old value.
                 groove.objects.put(new);
+                groove.objects_cache.upsert(new);
+            } else {
+                // TODO: Should we assert we actually updated something...?
+                return;
             }
 
             inline for (std.meta.fields(IndexTrees)) |field| {
                 const Helper = IndexTreeFieldHelperType(field.name);
-                const old_index = Helper.derive_index(&old);
+                const old_index = Helper.derive_index(old);
                 const new_index = Helper.derive_index(new);
 
                 // Only update the indexes that change.
                 if (!std.meta.eql(old_index, new_index)) {
                     if (old_index) |index| {
-                        const old_index_value = Helper.derive_value(&old, index);
+                        const old_index_value = Helper.derive_value(old, index);
                         @field(groove.indexes, field.name).remove(&old_index_value);
                     }
 
diff --git a/src/state_machine.zig b/src/state_machine.zig
index 282ae4486d..553396856f 100644
--- a/src/state_machine.zig
+++ b/src/state_machine.zig
@@ -876,8 +876,8 @@ pub fn StateMachineType(
                 dr_account_new.debits_posted += amount;
                 cr_account_new.credits_posted += amount;
             }
-            self.forest.grooves.accounts.upsert(&dr_account_new);
-            self.forest.grooves.accounts.upsert(&cr_account_new);
+            self.forest.grooves.accounts.update(&dr_account_new, dr_account);
+            self.forest.grooves.accounts.update(&cr_account_new, cr_account);
 
             self.commit_timestamp = t.timestamp;
             return .ok;
@@ -1006,8 +1006,8 @@ pub fn StateMachineType(
                 cr_account_new.credits_posted += amount;
             }
 
-            self.forest.grooves.accounts.upsert(&dr_account_new);
-            self.forest.grooves.accounts.upsert(&cr_account_new);
+            self.forest.grooves.accounts.update(&dr_account_new, dr_account);
+            self.forest.grooves.accounts.update(&cr_account_new, cr_account);
 
             self.commit_timestamp = t.timestamp;
             return .ok;
@@ -1399,11 +1399,13 @@ fn check(test_table: []const u8) !void {
                 assert(operation == null);
 
                 var account = context.state_machine.forest.grooves.accounts.get(b.account).?.*;
-                account.debits_pending = b.debits_pending;
-                account.debits_posted = b.debits_posted;
-                account.credits_pending = b.credits_pending;
-                account.credits_posted = b.credits_posted;
-                context.state_machine.forest.grooves.accounts.upsert(&account);
+                var account_new = account;
+
+                account_new.debits_pending = b.debits_pending;
+                account_new.debits_posted = b.debits_posted;
+                account_new.credits_pending = b.credits_pending;
+                account_new.credits_posted = b.credits_posted;
+                context.state_machine.forest.grooves.accounts.update(&account_new, &account);
             },
 
             .tick => |ticks| {
diff --git a/src/testing/state_machine.zig b/src/testing/state_machine.zig
index 05bc040134..bede504870 100644
--- a/src/testing/state_machine.zig
+++ b/src/testing/state_machine.zig
@@ -167,7 +167,7 @@ pub fn StateMachineType(
                     const thing = state_machine.forest.grooves.things.get(op);
                     assert(thing == null);
 
-                    state_machine.forest.grooves.things.upsert(&.{
+                    state_machine.forest.grooves.things.insert(&.{
                         .timestamp = timestamp,
                         .id = op,
                         .value = @as(u64, @truncate(vsr.checksum(input))),

From 65f21d655828852f9a82cad34981f0ed129a6805 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Mon, 23 Oct 2023 12:15:57 +0200
Subject: [PATCH 3/7] Groove: Remove stdx.equal_bytes from the hot path
 entirely

The expectation is that the state machine will call .update() when there is in
fact an update. We still check stdx.equal_bytes gated behind constants.verify.

We also double check key_from_value and that the update is not a tombstone,
too.
---
 src/lsm/groove.zig    | 30 ++++++++++++++++++------------
 src/state_machine.zig |  4 +++-
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/lsm/groove.zig b/src/lsm/groove.zig
index 37c22e72df..0c3fd3ddda 100644
--- a/src/lsm/groove.zig
+++ b/src/lsm/groove.zig
@@ -900,22 +900,28 @@ pub fn GrooveType(
             // Sanity check to ensure the caller didn't accidentally pass in an alias.
             assert(new != old);
 
-            // TODO: This will assert timestamp twice if `has_id` is false.
-            assert(@field(old, primary_field) == @field(new, primary_field));
+            if (has_id) {
+                assert(old.id == new.id);
+            }
             assert(old.timestamp == new.timestamp);
 
-            // The ID can't change, so no need to update the ID tree.
-            // Update the object tree entry if any of the fields (even ignored) are different.
-            if (!stdx.equal_bytes(Object, old, new)) {
-                // Unlike the index trees, the new and old values in the object tree share the
-                // same key. Therefore put() is sufficient to overwrite the old value.
-                groove.objects.put(new);
-                groove.objects_cache.upsert(new);
-            } else {
-                // TODO: Should we assert we actually updated something...?
-                return;
+            // The ID can't change, so no need to update the ID tree. Update the object tree entry
+            // if any of the fields (even ignored) are different. We assume the caller will pass in
+            // an object that has changes.
+            // Unlike the index trees, the new and old values in the object tree share the same
+            // key. Therefore put() is sufficient to overwrite the old value.
+            if (constants.verify) {
+                const tombstone = ObjectTreeHelpers(Object).tombstone;
+                const key_from_value = ObjectTreeHelpers(Object).key_from_value;
+
+                assert(!stdx.equal_bytes(Object, old, new));
+                assert(key_from_value(old) == key_from_value(new));
+                assert(!tombstone(old) and !tombstone(new));
             }
 
+            groove.objects.put(new);
+            groove.objects_cache.upsert(new);
+
             inline for (std.meta.fields(IndexTrees)) |field| {
                 const Helper = IndexTreeFieldHelperType(field.name);
                 const old_index = Helper.derive_index(old);
diff --git a/src/state_machine.zig b/src/state_machine.zig
index 553396856f..dbc2f10834 100644
--- a/src/state_machine.zig
+++ b/src/state_machine.zig
@@ -1405,7 +1405,9 @@ fn check(test_table: []const u8) !void {
                 account_new.debits_posted = b.debits_posted;
                 account_new.credits_pending = b.credits_pending;
                 account_new.credits_posted = b.credits_posted;
-                context.state_machine.forest.grooves.accounts.update(&account_new, &account);
+                if (!stdx.equal_bytes(Account, &account_new, &account)) {
+                    context.state_machine.forest.grooves.accounts.update(&account_new, &account);
+                }
             },
 
             .tick => |ticks| {

From 06bb1283bfae87270c1b5e6962a3fdcff7f764d7 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Mon, 23 Oct 2023 13:14:12 +0200
Subject: [PATCH 4/7] Groove: Make update() take a struct to avoid ambiguity
 around param order

* target first style for copies gets const protection, which we can't take
  advantage of here.
* There's no real convention, because this .update() method doesn't update
  a parameter with an existing one, but rather updates the groove based off
  of both.
* There's no performance cost to using a struct (asm generated on godbolt
  was identical and double checked on the benchmark).
---
 src/lsm/groove.zig    |  8 +++++++-
 src/state_machine.zig | 13 ++++++++-----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/lsm/groove.zig b/src/lsm/groove.zig
index 0c3fd3ddda..ed72e637fc 100644
--- a/src/lsm/groove.zig
+++ b/src/lsm/groove.zig
@@ -890,7 +890,13 @@ pub fn GrooveType(
 
         /// Update the value. Requires the old object to be provided.
         /// Update the object and index trees by diff'ing the old and new values.
-        pub fn update(groove: *Groove, new: *const Object, old: *const Object) void {
+        pub fn update(
+            groove: *Groove,
+            values: struct { old: *const Object, new: *const Object },
+        ) void {
+            const old = values.old;
+            const new = values.new;
+
             if (constants.verify) {
                 const old_from_cache = groove.objects_cache.get(@field(old, primary_field)).?;
                 assert(@field(old_from_cache, primary_field) == @field(old, primary_field));
diff --git a/src/state_machine.zig b/src/state_machine.zig
index dbc2f10834..aa343dc6ab 100644
--- a/src/state_machine.zig
+++ b/src/state_machine.zig
@@ -876,8 +876,8 @@ pub fn StateMachineType(
                 dr_account_new.debits_posted += amount;
                 cr_account_new.credits_posted += amount;
             }
-            self.forest.grooves.accounts.update(&dr_account_new, dr_account);
-            self.forest.grooves.accounts.update(&cr_account_new, cr_account);
+            self.forest.grooves.accounts.update(.{ .old = dr_account, .new = &dr_account_new });
+            self.forest.grooves.accounts.update(.{ .old = cr_account, .new = &cr_account_new });
 
             self.commit_timestamp = t.timestamp;
             return .ok;
@@ -1006,8 +1006,8 @@ pub fn StateMachineType(
                 cr_account_new.credits_posted += amount;
             }
 
-            self.forest.grooves.accounts.update(&dr_account_new, dr_account);
-            self.forest.grooves.accounts.update(&cr_account_new, cr_account);
+            self.forest.grooves.accounts.update(.{ .old = dr_account, .new = &dr_account_new });
+            self.forest.grooves.accounts.update(.{ .old = cr_account, .new = &cr_account_new });
 
             self.commit_timestamp = t.timestamp;
             return .ok;
@@ -1406,7 +1406,10 @@ fn check(test_table: []const u8) !void {
                 account_new.credits_pending = b.credits_pending;
                 account_new.credits_posted = b.credits_posted;
                 if (!stdx.equal_bytes(Account, &account_new, &account)) {
-                    context.state_machine.forest.grooves.accounts.update(&account_new, &account);
+                    context.state_machine.forest.grooves.accounts.update(.{
+                        .old = &account,
+                        .new = &account_new,
+                    });
                 }
             },
 

From c367d1dfdba783b8eb171cdfec5d9de0188517c5 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Mon, 23 Oct 2023 13:15:37 +0200
Subject: [PATCH 5/7] Tidy: add std hasUniqueRepresentation to banned list

---
 src/tidy.zig | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/tidy.zig b/src/tidy.zig
index 75e2f0d976..906a9bc656 100644
--- a/src/tidy.zig
+++ b/src/tidy.zig
@@ -64,6 +64,11 @@ fn banned(source: []const u8) ?[]const u8 {
     if (std.mem.indexOf(u8, source, "std." ++ "BoundedArray") != null) {
         return "use stdx." ++ "BoundedArray instead of std version";
     }
+
+    if (std.mem.indexOf(u8, source, "trait." ++ "hasUniqueRepresentation") != null) {
+        return "use stdx." ++ "hasUniqueRepresentation instead of std version";
+    }
+
     return null;
 }
 

From 0733fb7056a75578fdc13b3342105a98e1b28ee1 Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Tue, 24 Oct 2023 11:22:50 +0200
Subject: [PATCH 6/7] LSM: Fuzz fix, review changes

---
 src/lsm/forest_fuzz.zig |  18 +++---
 src/lsm/groove.zig      |  18 ++++--
 src/state_machine.zig   |   8 +--
 src/stdx.zig            | 125 +++++++++++++++++++++++++++++++++++++---
 src/tidy.zig            |   2 +-
 5 files changed, 145 insertions(+), 26 deletions(-)

diff --git a/src/lsm/forest_fuzz.zig b/src/lsm/forest_fuzz.zig
index 385aa3cec9..98f1a19358 100644
--- a/src/lsm/forest_fuzz.zig
+++ b/src/lsm/forest_fuzz.zig
@@ -283,13 +283,17 @@ const Environment = struct {
         }
     }
 
-    fn put_account(env: *Environment, a: *const Account) void {
-        env.forest.grooves.accounts.upsert(a);
+    fn put_account(env: *Environment, a: *const Account, maybe_old: ?*const Account) void {
+        if (maybe_old) |old| {
+            env.forest.grooves.accounts.update(.{ .old = old, .new = a });
+        } else {
+            env.forest.grooves.accounts.insert(a);
+        }
     }
 
-    fn get_account(env: *Environment, id: u128) ?Account {
+    fn get_account(env: *Environment, id: u128) ?*const Account {
         const account = env.forest.grooves.accounts.get(id) orelse return null;
-        return account.*;
+        return account;
     }
 
     // The forest should behave like a simple key-value data-structure.
@@ -441,8 +445,7 @@ const Environment = struct {
                             env.forest.grooves.accounts.objects_cache.options.map_value_count_max;
 
                         if (log_index % groove_map_value_count_max == 0) {
-                            env.forest.grooves.accounts_immutable.objects_cache.compact();
-                            env.forest.grooves.accounts_mutable.objects_cache.compact();
+                            env.forest.grooves.accounts.objects_cache.compact();
                         }
                     }
                 }
@@ -463,7 +466,8 @@ const Environment = struct {
             .put_account => |put| {
                 // The forest requires prefetch before put.
                 try env.prefetch_account(put.account.id);
-                env.put_account(&put.account);
+                const lsm_account = env.get_account(put.account.id);
+                env.put_account(&put.account, lsm_account);
                 try model.put_account(&put.account, put.op);
             },
             .get_account => |id| {
diff --git a/src/lsm/groove.zig b/src/lsm/groove.zig
index ed72e637fc..b841176874 100644
--- a/src/lsm/groove.zig
+++ b/src/lsm/groove.zig
@@ -899,8 +899,12 @@ pub fn GrooveType(
 
             if (constants.verify) {
                 const old_from_cache = groove.objects_cache.get(@field(old, primary_field)).?;
-                assert(@field(old_from_cache, primary_field) == @field(old, primary_field));
-                assert(old_from_cache.timestamp == old.timestamp);
+
+                // While all that's actually required is that the _contents_ of the old_from_cache
+                // and old objects are identical, in current usage they're always the same piece of
+                // memory. We'll assert that for now, and this can be weakened in future if
+                // required.
+                assert(old_from_cache == old);
             }
 
             // Sanity check to ensure the caller didn't accidentally pass in an alias.
@@ -925,9 +929,6 @@ pub fn GrooveType(
                 assert(!tombstone(old) and !tombstone(new));
             }
 
-            groove.objects.put(new);
-            groove.objects_cache.upsert(new);
-
             inline for (std.meta.fields(IndexTrees)) |field| {
                 const Helper = IndexTreeFieldHelperType(field.name);
                 const old_index = Helper.derive_index(old);
@@ -946,6 +947,13 @@ pub fn GrooveType(
                     }
                 }
             }
+
+            // Putting the objects_cache upsert after the index tree updates is critical:
+            // We diff the old and new objects, but the old object will be a pointer into the
+            // objects_cache. If we upsert first, old.* == new.* and no secondary indexes will
+            // be updated!
+            groove.objects_cache.upsert(new);
+            groove.objects.put(new);
         }
 
         /// Asserts that the object with the given PrimaryKey exists.
diff --git a/src/state_machine.zig b/src/state_machine.zig
index aa343dc6ab..70ffffc4fa 100644
--- a/src/state_machine.zig
+++ b/src/state_machine.zig
@@ -1398,16 +1398,16 @@ fn check(test_table: []const u8) !void {
             .setup => |b| {
                 assert(operation == null);
 
-                var account = context.state_machine.forest.grooves.accounts.get(b.account).?.*;
-                var account_new = account;
+                var account = context.state_machine.forest.grooves.accounts.get(b.account).?;
+                var account_new = account.*;
 
                 account_new.debits_pending = b.debits_pending;
                 account_new.debits_posted = b.debits_posted;
                 account_new.credits_pending = b.credits_pending;
                 account_new.credits_posted = b.credits_posted;
-                if (!stdx.equal_bytes(Account, &account_new, &account)) {
+                if (!stdx.equal_bytes(Account, &account_new, account)) {
                     context.state_machine.forest.grooves.accounts.update(.{
-                        .old = &account,
+                        .old = account,
                         .new = &account_new,
                     });
                 }
diff --git a/src/stdx.zig b/src/stdx.zig
index 7cee678147..a196147ff2 100644
--- a/src/stdx.zig
+++ b/src/stdx.zig
@@ -242,7 +242,7 @@ else
 ///   - `T` doesn't have any non-deterministic padding,
 ///   - `T` doesn't embed any pointers.
 pub fn equal_bytes(comptime T: type, a: *const T, b: *const T) bool {
-    comptime assert(hasUniqueRepresentation(T));
+    comptime assert(has_unique_representation(T));
     comptime assert(!has_pointers(T));
     comptime assert(@sizeOf(T) * 8 == @bitSizeOf(T));
 
@@ -339,7 +339,7 @@ test no_padding {
 pub inline fn hash_inline(value: anytype) u64 {
     comptime {
         assert(no_padding(@TypeOf(value)));
-        assert(hasUniqueRepresentation(@TypeOf(value)));
+        assert(has_unique_representation(@TypeOf(value)));
     }
     return low_level_hash(0, switch (@typeInfo(@TypeOf(value))) {
         .Struct, .Int => std.mem.asBytes(&value),
@@ -524,7 +524,7 @@ pub fn fstatfs(fd: i32, statfs_buf: *StatFs) usize {
 // TODO(Zig): https://github.com/ziglang/zig/issues/17592.
 /// True if every value of the type `T` has a unique bit pattern representing it.
 /// In other words, `T` has no unused bits and no padding.
-pub fn hasUniqueRepresentation(comptime T: type) bool {
+pub fn has_unique_representation(comptime T: type) bool {
     switch (@typeInfo(T)) {
         else => return false, // TODO can we know if it's true for some of these types ?
 
@@ -540,26 +540,133 @@ pub fn hasUniqueRepresentation(comptime T: type) bool {
 
         .Pointer => |info| return info.size != .Slice,
 
-        .Array => |info| return comptime hasUniqueRepresentation(info.child),
+        .Array => |info| return comptime has_unique_representation(info.child),
 
         .Struct => |info| {
-            // Packed structs are always unique.
-            if (info.backing_integer != null) {
-                return true;
+            // Only consider packed structs unique if they are byte aligned.
+            if (info.backing_integer) |backing_integer| {
+                return @sizeOf(@Type(.{ .Struct = info })) * 8 == @bitSizeOf(backing_integer);
             }
 
             var sum_size = @as(usize, 0);
 
             inline for (info.fields) |field| {
                 const FieldType = field.type;
-                if (comptime !hasUniqueRepresentation(FieldType)) return false;
+                if (comptime !has_unique_representation(FieldType)) return false;
                 sum_size += @sizeOf(FieldType);
             }
 
             return @sizeOf(T) == sum_size;
         },
 
-        .Vector => |info| return comptime hasUniqueRepresentation(info.child) and
+        .Vector => |info| return comptime has_unique_representation(info.child) and
             @sizeOf(T) == @sizeOf(info.child) * info.len,
     }
 }
+
+// Test vectors mostly from upstream, with some added to test the packed struct case.
+test "has_unique_representation" {
+    const TestStruct1 = struct {
+        a: u32,
+        b: u32,
+    };
+
+    try std.testing.expect(has_unique_representation(TestStruct1));
+
+    const TestStruct2 = struct {
+        a: u32,
+        b: u16,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestStruct2));
+
+    const TestStruct3 = struct {
+        a: u32,
+        b: u32,
+    };
+
+    try std.testing.expect(has_unique_representation(TestStruct3));
+
+    const TestStruct4 = struct { a: []const u8 };
+
+    try std.testing.expect(!has_unique_representation(TestStruct4));
+
+    const TestStruct5 = struct { a: TestStruct4 };
+
+    try std.testing.expect(!has_unique_representation(TestStruct5));
+
+    const TestStruct6 = packed struct {
+        a: u32,
+        b: u31,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestStruct6));
+
+    const TestStruct7 = struct {
+        a: u64,
+        b: TestStruct6,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestStruct7));
+
+    const TestStruct8 = packed struct {
+        a: u32,
+        b: u32,
+    };
+
+    try std.testing.expect(has_unique_representation(TestStruct8));
+
+    const TestStruct9 = struct {
+        a: u64,
+        b: TestStruct8,
+    };
+
+    try std.testing.expect(has_unique_representation(TestStruct9));
+
+    const TestStruct10 = packed struct {
+        a: TestStruct8,
+        b: TestStruct8,
+    };
+
+    try std.testing.expect(has_unique_representation(TestStruct10));
+
+    const TestUnion1 = packed union {
+        a: u32,
+        b: u16,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestUnion1));
+
+    const TestUnion2 = extern union {
+        a: u32,
+        b: u16,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestUnion2));
+
+    const TestUnion3 = union {
+        a: u32,
+        b: u16,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestUnion3));
+
+    const TestUnion4 = union(enum) {
+        a: u32,
+        b: u16,
+    };
+
+    try std.testing.expect(!has_unique_representation(TestUnion4));
+
+    inline for ([_]type{ i0, u8, i16, u32, i64 }) |T| {
+        try std.testing.expect(has_unique_representation(T));
+    }
+    inline for ([_]type{ i1, u9, i17, u33, i24 }) |T| {
+        try std.testing.expect(!has_unique_representation(T));
+    }
+
+    try std.testing.expect(!has_unique_representation([]u8));
+    try std.testing.expect(!has_unique_representation([]const u8));
+
+    try std.testing.expect(has_unique_representation(@Vector(4, u16)));
+}
diff --git a/src/tidy.zig b/src/tidy.zig
index 906a9bc656..99ae89d0df 100644
--- a/src/tidy.zig
+++ b/src/tidy.zig
@@ -66,7 +66,7 @@ fn banned(source: []const u8) ?[]const u8 {
     }
 
     if (std.mem.indexOf(u8, source, "trait." ++ "hasUniqueRepresentation") != null) {
-        return "use stdx." ++ "hasUniqueRepresentation instead of std version";
+        return "use stdx." ++ "has_unique_representation instead of std version";
     }
 
     return null;

From e81a08730c86ad9bd27bc07e965a6132db443c3a Mon Sep 17 00:00:00 2001
From: Federico Lorenzi <florenzi@gmail.com>
Date: Tue, 24 Oct 2023 13:18:02 +0200
Subject: [PATCH 7/7] LSM: Review changes

---
 src/lsm/groove.zig | 8 +++-----
 src/stdx.zig       | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/lsm/groove.zig b/src/lsm/groove.zig
index b841176874..46d2ca7b72 100644
--- a/src/lsm/groove.zig
+++ b/src/lsm/groove.zig
@@ -910,9 +910,7 @@ pub fn GrooveType(
             // Sanity check to ensure the caller didn't accidentally pass in an alias.
             assert(new != old);
 
-            if (has_id) {
-                assert(old.id == new.id);
-            }
+            if (has_id) assert(old.id == new.id);
             assert(old.timestamp == new.timestamp);
 
             // The ID can't change, so no need to update the ID tree. Update the object tree entry
@@ -950,8 +948,8 @@ pub fn GrooveType(
 
             // Putting the objects_cache upsert after the index tree updates is critical:
             // We diff the old and new objects, but the old object will be a pointer into the
-            // objects_cache. If we upsert first, old.* == new.* and no secondary indexes will
-            // be updated!
+            // objects_cache. If we upsert first, there's a high chance old.* == new.* (always,
+            // unless old comes from the stash) and no secondary indexes will be updated!
             groove.objects_cache.upsert(new);
             groove.objects.put(new);
         }
diff --git a/src/stdx.zig b/src/stdx.zig
index a196147ff2..e7ae2ec155 100644
--- a/src/stdx.zig
+++ b/src/stdx.zig
@@ -545,7 +545,7 @@ pub fn has_unique_representation(comptime T: type) bool {
         .Struct => |info| {
             // Only consider packed structs unique if they are byte aligned.
             if (info.backing_integer) |backing_integer| {
-                return @sizeOf(@Type(.{ .Struct = info })) * 8 == @bitSizeOf(backing_integer);
+                return @sizeOf(T) * 8 == @bitSizeOf(backing_integer);
             }
 
             var sum_size = @as(usize, 0);