Skip to content

Commit f906be6

Browse files
authored
Fixed Nullable value-type fields in Views in C# module causing excessive logging (#3949)
# Description of Changes This PR fixes a C# codegen performance/behavior issue triggered by views that include nullable value-type fields (e.g. `DbVector2?`), as reported in #3914. * Updated the C# BSATN code generator to emit non-boxing equality for `Nullable<T>` members by using `System.Nullable.Equals(a, b)` rather than `a.Equals(b)` (which can box and cause excessive host logging/work in view diff/equality paths). This causes code generation to change from something like: ``` csharp // From: var ___eqNullableIntField = this.NullableIntField.Equals(that.NullableIntField); // To: var ___eqNullableIntField = System.Nullable.Equals( this.NullableIntField, that.NullableIntField ); ``` * Added a regression scenario to the C# regression-test module that exercises the problematic pattern: * A table containing a nullable struct field (`DbVector2? Pos`). * A public view that returns rows containing that nullable field. * A reducer to mutate the nullable field from `some` → `none` → `some` to force view re-evaluation and diffing. * Updated the regression-test client to: * Subscribe to the new view. * Validate that the view evaluates successfully and contains the expected rows. * Call the reducer and validate view state after updates. * Fail the test if view evaluation/diffing produces errors. # API and ABI breaking changes None. * No changes to public SpacetimeDB schema or wire format semantics. * Changes are limited to generated equality code and regression tests. # Expected complexity level and risk 2 - Low * The codegen change is small and localized (special-casing `System.Nullable<T>` equality generation). * Risk is primarily around subtle behavior differences in equality for nullable value types; however, `System.Nullable.Equals` matches the expected semantics and avoids boxing. * Regression tests specifically cover the nullable-struct-in-view scenario to guard against regressions. # Testing <!-- Describe any testing you've done, and any testing you'd like your reviewers to do, so that you're confident that all the changes work as expected! --> - [X] Ran the C# regression test suite via `run-regression-tests.sh` and confirmed no errors.
1 parent 33e3f24 commit f906be6

17 files changed

Lines changed: 409 additions & 11 deletions

File tree

crates/bindings-csharp/BSATN.Codegen/Type.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ public static TypeUse Parse(ISymbol member, ITypeSymbol typeSymbol, DiagReporter
9595
typeInfo,
9696
Parse(member, named.TypeArguments[0], diag)
9797
),
98+
"System.Nullable<T>" => new NullableUse(type, typeInfo),
9899
_ => named.IsValueType
99100
? (
100101
named.TypeKind == Microsoft.CodeAnalysis.TypeKind.Enum
@@ -182,6 +183,24 @@ public override string GetHashCodeStatement(string inVar, string outVar, int lev
182183
$"var {outVar} = {inVar}.GetHashCode();";
183184
}
184185

186+
/// <summary>
187+
/// A use of a nullable value type (e.g. <c>int?</c>, <c>MyStruct?</c>).
188+
/// </summary>
189+
/// <param name="Type"></param>
190+
/// <param name="TypeInfo"></param>
191+
public record NullableUse(string Type, string TypeInfo) : TypeUse(Type, TypeInfo)
192+
{
193+
public override string EqualsStatement(
194+
string inVar1,
195+
string inVar2,
196+
string outVar,
197+
int level = 0
198+
) => $"var {outVar} = System.Nullable.Equals({inVar1}, {inVar2});";
199+
200+
public override string GetHashCodeStatement(string inVar, string outVar, int level = 0) =>
201+
$"var {outVar} = {inVar}.GetHashCode();";
202+
}
203+
185204
/// <summary>
186205
/// A use of a reference type.
187206
/// </summary>

crates/bindings-csharp/Codegen.Tests/fixtures/client/snapshots/Type#PublicTable.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,10 @@ public bool Equals(PublicTable that)
283283
}
284284
}
285285
}
286-
var ___eqNullableValueField = this.NullableValueField.Equals(that.NullableValueField);
286+
var ___eqNullableValueField = System.Nullable.Equals(
287+
this.NullableValueField,
288+
that.NullableValueField
289+
);
287290
var ___eqNullableReferenceField =
288291
this.NullableReferenceField == null
289292
? that.NullableReferenceField == null

crates/bindings-csharp/Codegen.Tests/fixtures/diag/snapshots/Module#TestDefaultFieldValues.verified.cs

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/bindings-csharp/Codegen.Tests/fixtures/diag/snapshots/Module#TestUniqueNotEquatable.verified.cs

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/bindings-csharp/Codegen.Tests/fixtures/server/snapshots/Module#PublicTable.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,10 @@ public bool Equals(PublicTable that)
307307
}
308308
}
309309
}
310-
var ___eqNullableValueField = this.NullableValueField.Equals(that.NullableValueField);
310+
var ___eqNullableValueField = System.Nullable.Equals(
311+
this.NullableValueField,
312+
that.NullableValueField
313+
);
311314
var ___eqNullableReferenceField =
312315
this.NullableReferenceField == null
313316
? that.NullableReferenceField == null

crates/bindings-csharp/Codegen.Tests/fixtures/server/snapshots/Type#CustomClass.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,10 @@ public bool Equals(CustomClass? that)
9797
this.StringField == null
9898
? that.StringField == null
9999
: this.StringField.Equals(that.StringField);
100-
var ___eqNullableIntField = this.NullableIntField.Equals(that.NullableIntField);
100+
var ___eqNullableIntField = System.Nullable.Equals(
101+
this.NullableIntField,
102+
that.NullableIntField
103+
);
101104
var ___eqNullableStringField =
102105
this.NullableStringField == null
103106
? that.NullableStringField == null

crates/bindings-csharp/Codegen.Tests/fixtures/server/snapshots/Type#CustomNestedClass.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,10 @@ public bool Equals(CustomNestedClass? that)
147147
? that.NestedNullableClass == null
148148
: this.NestedNullableClass.Equals(that.NestedNullableClass);
149149
var ___eqNestedEnum = this.NestedEnum == that.NestedEnum;
150-
var ___eqNestedNullableEnum = this.NestedNullableEnum.Equals(that.NestedNullableEnum);
150+
var ___eqNestedNullableEnum = System.Nullable.Equals(
151+
this.NestedNullableEnum,
152+
that.NestedNullableEnum
153+
);
151154
var ___eqNestedTaggedEnum =
152155
this.NestedTaggedEnum == null
153156
? that.NestedTaggedEnum == null

crates/bindings-csharp/Codegen.Tests/fixtures/server/snapshots/Type#CustomRecord.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,10 @@ public bool Equals(CustomRecord? that)
9797
this.StringField == null
9898
? that.StringField == null
9999
: this.StringField.Equals(that.StringField);
100-
var ___eqNullableIntField = this.NullableIntField.Equals(that.NullableIntField);
100+
var ___eqNullableIntField = System.Nullable.Equals(
101+
this.NullableIntField,
102+
that.NullableIntField
103+
);
101104
var ___eqNullableStringField =
102105
this.NullableStringField == null
103106
? that.NullableStringField == null

crates/bindings-csharp/Codegen.Tests/fixtures/server/snapshots/Type#CustomStruct.verified.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,10 @@ public bool Equals(CustomStruct that)
9494
this.StringField == null
9595
? that.StringField == null
9696
: this.StringField.Equals(that.StringField);
97-
var ___eqNullableIntField = this.NullableIntField.Equals(that.NullableIntField);
97+
var ___eqNullableIntField = System.Nullable.Equals(
98+
this.NullableIntField,
99+
that.NullableIntField
100+
);
98101
var ___eqNullableStringField =
99102
this.NullableStringField == null
100103
? that.NullableStringField == null

sdks/csharp/examples~/regression-tests/client/Program.cs

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/// Regression tests run with a live server.
1+
/// Regression tests run with a live server.
22
/// To run these, run a local SpacetimeDB via `spacetime start`,
33
/// then in a separate terminal run `tools~/run-regression-tests.sh PATH_TO_SPACETIMEDB_REPO_CHECKOUT`.
44
/// This is done on CI in .github/workflows/test.yml.
@@ -65,6 +65,7 @@ void OnConnected(DbConnection conn, Identity identity, string authToken)
6565
"SELECT * FROM players_at_level_one",
6666
"SELECT * FROM my_table",
6767
"SELECT * FROM Admins",
68+
"SELECT * FROM nullable_vec_view",
6869
]);
6970

7071
// If testing against Rust, the indexed parameter will need to be changed to: ulong indexed
@@ -87,6 +88,21 @@ void OnConnected(DbConnection conn, Identity identity, string authToken)
8788
Log.Info($"Got OnUnhandledReducerError: {exception}");
8889
waiting--;
8990
ValidateBTreeIndexes(ctx);
91+
ValidateNullableVecView(ctx);
92+
};
93+
94+
conn.Reducers.OnSetNullableVec += (ReducerEventContext ctx, uint id, bool hasPos, int x, int y) =>
95+
{
96+
Log.Info("Got SetNullableVec callback");
97+
waiting--;
98+
if (id == 1)
99+
{
100+
ValidateNullableVecView(ctx, hasPos, x, y);
101+
}
102+
else
103+
{
104+
ValidateNullableVecView(ctx);
105+
}
90106
};
91107
}
92108

@@ -113,6 +129,44 @@ void ValidateBTreeIndexes(IRemoteDbContext conn)
113129
Log.Debug(" Indexes are good.");
114130
}
115131

132+
void ValidateNullableVecView(
133+
IRemoteDbContext conn,
134+
bool? expectedHasPos = null,
135+
int expectedX = 0,
136+
int expectedY = 0
137+
)
138+
{
139+
Log.Debug("Checking nullable vec view...");
140+
Debug.Assert(conn.Db.NullableVecView != null, "conn.Db.NullableVecView != null");
141+
Debug.Assert(
142+
conn.Db.NullableVecView.Count >= 2,
143+
$"conn.Db.NullableVecView.Count = {conn.Db.NullableVecView.Count}"
144+
);
145+
146+
var rows = conn.Db.NullableVecView.Iter().ToList();
147+
Debug.Assert(rows.Any(r => r.Id == 1));
148+
Debug.Assert(rows.Any(r => r.Id == 2));
149+
150+
var remoteRows = conn.Db.NullableVecView.RemoteQuery("WHERE Id = 1").Result;
151+
Debug.Assert(remoteRows != null && remoteRows.Length == 1);
152+
Debug.Assert(remoteRows[0].Id == 1);
153+
154+
if (expectedHasPos is bool hasPos)
155+
{
156+
var row1 = rows.First(r => r.Id == 1);
157+
if (!hasPos)
158+
{
159+
Debug.Assert(row1.Pos == null, "Expected NullableVecView row 1 Pos == null");
160+
}
161+
else
162+
{
163+
Debug.Assert(row1.Pos != null, "Expected NullableVecView row 1 Pos != null");
164+
Debug.Assert(row1.Pos.X == expectedX, $"Expected row1.Pos.X == {expectedX}, got {row1.Pos.X}");
165+
Debug.Assert(row1.Pos.Y == expectedY, $"Expected row1.Pos.Y == {expectedY}, got {row1.Pos.Y}");
166+
}
167+
}
168+
}
169+
116170
void OnSubscriptionApplied(SubscriptionEventContext context)
117171
{
118172
applied = true;
@@ -160,6 +214,8 @@ void OnSubscriptionApplied(SubscriptionEventContext context)
160214
Debug.Assert(context.Db.Admins != null, "context.Db.Admins != null");
161215
Debug.Assert(context.Db.Admins.Count > 0, $"context.Db.Admins.Count = {context.Db.Admins.Count}");
162216

217+
ValidateNullableVecView(context, expectedHasPos: true, expectedX: 1, expectedY: 2);
218+
163219
Log.Debug("Calling Iter on View");
164220
var viewIterRows = context.Db.MyPlayer.Iter();
165221
var expectedPlayer = new Player
@@ -230,6 +286,14 @@ void OnSubscriptionApplied(SubscriptionEventContext context)
230286
Debug.Assert(anonViewRemoteQueryRows != null && anonViewRemoteQueryRows.Result.Length > 0);
231287
Debug.Assert(anonViewRemoteQueryRows.Result.First().Equals(expectedPlayerAndLevel));
232288

289+
Log.Debug("Calling SetNullableVec (null)");
290+
waiting++;
291+
context.Reducers.SetNullableVec(1, false, 0, 0);
292+
293+
Log.Debug("Calling SetNullableVec (some)");
294+
waiting++;
295+
context.Reducers.SetNullableVec(1, true, 7, 8);
296+
233297
// Procedures tests
234298
Log.Debug("Calling InsertWithTxRollback");
235299
waiting++;

0 commit comments

Comments
 (0)