Merge branch 'sm/matrix-stress-stFWW' into sm/matrix-rollback

Author: anthony-murphy

packages/dds/matrix/src/matrix.ts

@@ -205,6 +205,15 @@ export interface ISharedMatrix<T = any>
 	switchSetCellPolicy(): void;
 }
 
+type FirstWriterWinsPolicy =
+	| { state: "off" }
+	| { state: "local" }
+	| {
+			state: "on";
+			switchOpSeqNumber: number;
+			cellLastWriteTracker: SparseArray2D<CellLastWriteTrackerItem>;
+	  };
+
 /**
  * A SharedMatrix holds a rectangular 2D array of values.  Supported operations
  * include setting values and inserting/removing rows and columns.
@@ -245,10 +254,10 @@ export class SharedMatrix<T = any>
 
 	private cells = new SparseArray2D<MatrixItem<T>>(); // Stores cell values.
 	private readonly pending = new SparseArray2D<number[]>(); // Tracks pending writes.
-	private cellLastWriteTracker = new SparseArray2D<CellLastWriteTrackerItem>(); // Tracks last writes sequence number and clientId in a cell.
-	// Tracks the seq number of Op at which policy switch happens from Last Write Win to First Write Win.
-	private setCellLwwToFwwPolicySwitchOpSeqNumber: number;
-	private userSwitchedSetCellPolicy = false; // Set to true when the user calls switchPolicy.
+
+	private fwwPolicy: FirstWriterWinsPolicy = {
+		state: "off",
+	};
 
 	// Used to track if there is any reentrancy in setCell code.
 	private reentrantCount: number = 0;
@@ -268,7 +277,6 @@ export class SharedMatrix<T = any>
 	) {
 		super(id, runtime, attributes, "fluid_matrix_");
 
-		this.setCellLwwToFwwPolicySwitchOpSeqNumber = -1;
 		this.rows = new PermutationVector(
 			SnapshotPath.rows,
 			this.logger,
@@ -334,7 +342,7 @@ export class SharedMatrix<T = any>
 	}
 
 	public isSetCellConflictResolutionPolicyFWW(): boolean {
-		return this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1 || this.userSwitchedSetCellPolicy;
+		return this.fwwPolicy.state !== "off";
 	}
 
 	public getCell(row: number, col: number): MatrixItem<T> {
@@ -472,8 +480,7 @@ export class SharedMatrix<T = any>
 			row,
 			col,
 			value,
-			fwwMode:
-				this.userSwitchedSetCellPolicy || this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1,
+			fwwMode: this.fwwPolicy.state !== "off",
 		};
 
 		const rowsRef = this.createOpMetadataLocalRef(this.rows, row, localSeq);
@@ -673,15 +680,29 @@ export class SharedMatrix<T = any>
 			SnapshotPath.cols,
 			this.cols.summarize(this.runtime, this.handle, serializer),
 		);
-		const artifactsToSummarize = [
-			this.cells.snapshot(),
-			this.pending.snapshot(),
-			this.setCellLwwToFwwPolicySwitchOpSeqNumber,
-		];
+		const artifactsToSummarize: (
+			| undefined
+			| number
+			| ReturnType<SparseArray2D<MatrixItem<T> | number>["snapshot"]>
+		)[] = [this.cells.snapshot(), this.pending.snapshot()];
 
 		// Only need to store it in the snapshot if we have switched the policy already.
-		if (this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1) {
-			artifactsToSummarize.push(this.cellLastWriteTracker.snapshot());
+		if (this.fwwPolicy.state === "on") {
+			artifactsToSummarize.push(
+				this.fwwPolicy.switchOpSeqNumber,
+				this.fwwPolicy.cellLastWriteTracker.snapshot(),
+			);
+		} else {
+			// back-compat:  used -1 for disabled
+			artifactsToSummarize.push(
+				-1,
+				/*
+				 * we should set undefined in place of cellLastWriteTracker to ensure the number of array entries is consistent.
+				 * Doing that currently breaks snapshot tests. Its is probably fine, but if new elements are ever added, we need
+				 * ensure undefined is also set.
+				 */
+				// undefined
+			);
 		}
 		builder.addBlob(
 			SnapshotPath.cells,
@@ -798,10 +819,6 @@ export class SharedMatrix<T = any>
 			this.rows.removeLocalReferencePosition(rowsRef);
 			this.cols.removeLocalReferencePosition(colsRef);
 			if (row !== undefined && col !== undefined && row >= 0 && col >= 0) {
-				const lastCellModificationDetails = this.cellLastWriteTracker.getCell(
-					rowHandle,
-					colHandle,
-				);
 				const pending = this.pending.getCell(rowHandle, colHandle);
 				assert(pending !== undefined, "local operation must have a pending array");
 				const localSeqIndex = pending.indexOf(localSeq);
@@ -813,9 +830,9 @@ export class SharedMatrix<T = any>
 				// otherwise raise conflict. We want to check the current mode here and not that
 				// whether op was made in FWW or not.
 				if (
-					this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 ||
-					lastCellModificationDetails === undefined ||
-					referenceSeqNumber >= lastCellModificationDetails.seqNum
+					this.fwwPolicy.state !== "on" ||
+					referenceSeqNumber >=
+						(this.fwwPolicy.cellLastWriteTracker.getCell(rowHandle, colHandle)?.seqNum ?? 0)
 				) {
 					this.sendSetCellOp(
 						row,
@@ -876,11 +893,21 @@ export class SharedMatrix<T = any>
 			];
 
 			this.cells = SparseArray2D.load(cellData);
-			this.setCellLwwToFwwPolicySwitchOpSeqNumber =
-				setCellLwwToFwwPolicySwitchOpSeqNumber ?? -1;
-			if (cellLastWriteTracker !== undefined) {
-				this.cellLastWriteTracker = SparseArray2D.load(cellLastWriteTracker);
-			}
+			// back-compat:  used -1 for disabled, also may not exist
+			const switchOpSeqNumber =
+				setCellLwwToFwwPolicySwitchOpSeqNumber === -1
+					? undefined
+					: (setCellLwwToFwwPolicySwitchOpSeqNumber ?? undefined);
+			this.fwwPolicy =
+				switchOpSeqNumber === undefined
+					? {
+							state: "off",
+						}
+					: {
+							state: "on",
+							switchOpSeqNumber,
+							cellLastWriteTracker: SparseArray2D.load(cellLastWriteTracker),
+						};
 		} catch (error) {
 			this.logger.sendErrorEvent({ eventName: "MatrixLoadFailed" }, error);
 		}
@@ -896,11 +923,11 @@ export class SharedMatrix<T = any>
 		message: ISequencedDocumentMessage,
 	): boolean {
 		assert(
-			this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1,
+			this.fwwPolicy.state === "on",
 			0x85f /* should be in Fww mode when calling this method */,
 		);
 		assert(message.clientId !== null, 0x860 /* clientId should not be null */);
-		const lastCellModificationDetails = this.cellLastWriteTracker.getCell(
+		const lastCellModificationDetails = this.fwwPolicy.cellLastWriteTracker.getCell(
 			rowHandle,
 			colHandle,
 		);
@@ -949,11 +976,13 @@ export class SharedMatrix<T = any>
 				);
 
 				const { row, col, value, fwwMode } = contents;
-				const isPreviousSetCellPolicyModeFWW =
-					this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1;
 				// If this is the first op notifying us of the policy change, then set the policy change seq number.
-				if (this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 && fwwMode === true) {
-					this.setCellLwwToFwwPolicySwitchOpSeqNumber = msg.sequenceNumber;
+				if (fwwMode === true && this.fwwPolicy.state !== "on") {
+					this.fwwPolicy = {
+						state: "on",
+						switchOpSeqNumber: msg.sequenceNumber,
+						cellLastWriteTracker: new SparseArray2D(),
+					};
 				}
 
 				assert(msg.clientId !== null, 0x861 /* clientId should not be null!! */);
@@ -971,17 +1000,15 @@ export class SharedMatrix<T = any>
 
 					// if there are no more pending entries, the current must be the latest
 					//
-					const isLatestPendingOp = pending.length === 0;
 					this.rows.removeLocalReferencePosition(rowsRef);
 					this.cols.removeLocalReferencePosition(colsRef);
 					// If policy is switched and cell should be modified too based on policy, then update the tracker.
 					// If policy is not switched, then also update the tracker in case it is the latest.
 					if (
-						(this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1 &&
-							this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) ||
-						(this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1 && isLatestPendingOp)
+						this.fwwPolicy.state === "on" &&
+						this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)
 					) {
-						this.cellLastWriteTracker.setCell(rowHandle, colHandle, {
+						this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, {
 							seqNum: msg.sequenceNumber,
 							clientId: msg.clientId,
 						});
@@ -999,17 +1026,14 @@ export class SharedMatrix<T = any>
 								isHandleValid(rowHandle) && isHandleValid(colHandle),
 								0x022 /* "SharedMatrix row and/or col handles are invalid!" */,
 							);
-							if (this.setCellLwwToFwwPolicySwitchOpSeqNumber > -1) {
+							if (this.fwwPolicy.state === "on") {
 								// If someone tried to Overwrite the cell value or first write on this cell or
 								// same client tried to modify the cell or if the previous mode was LWW, then we need to still
 								// overwrite the cell and raise conflict if we have pending changes as our change is going to be lost.
-								if (
-									!isPreviousSetCellPolicyModeFWW ||
-									this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)
-								) {
+								if (this.shouldSetCellBasedOnFWW(rowHandle, colHandle, msg)) {
 									const previousValue = this.cells.getCell(rowHandle, colHandle);
 									this.cells.setCell(rowHandle, colHandle, value);
-									this.cellLastWriteTracker.setCell(rowHandle, colHandle, {
+									this.fwwPolicy.cellLastWriteTracker.setCell(rowHandle, colHandle, {
 										seqNum: msg.sequenceNumber,
 										clientId: msg.clientId,
 									});
@@ -1034,10 +1058,6 @@ export class SharedMatrix<T = any>
 								// If there is a pending (unACKed) local write to the same cell, skip the current op
 								// since it "happened before" the pending write.
 								this.cells.setCell(rowHandle, colHandle, value);
-								this.cellLastWriteTracker.setCell(rowHandle, colHandle, {
-									seqNum: msg.sequenceNumber,
-									clientId: msg.clientId,
-								});
 								for (const consumer of this.consumers.values()) {
 									consumer.cellsChanged(adjustedRow, adjustedCol, 1, 1, this);
 								}
@@ -1115,25 +1135,37 @@ export class SharedMatrix<T = any>
 		for (const rowHandle of rowHandles) {
 			this.cells.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1);
 			this.pending.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1);
-			this.cellLastWriteTracker.clearRows(/* rowStart: */ rowHandle, /* rowCount: */ 1);
+			if (this.fwwPolicy.state === "on") {
+				this.fwwPolicy.cellLastWriteTracker?.clearRows(
+					/* rowStart: */ rowHandle,
+					/* rowCount: */ 1,
+				);
+			}
 		}
 	};
 
 	private readonly onColHandlesRecycled = (colHandles: Handle[]): void => {
 		for (const colHandle of colHandles) {
 			this.cells.clearCols(/* colStart: */ colHandle, /* colCount: */ 1);
 			this.pending.clearCols(/* colStart: */ colHandle, /* colCount: */ 1);
-			this.cellLastWriteTracker.clearCols(/* colStart: */ colHandle, /* colCount: */ 1);
+			if (this.fwwPolicy.state === "on") {
+				this.fwwPolicy.cellLastWriteTracker?.clearCols(
+					/* colStart: */ colHandle,
+					/* colCount: */ 1,
+				);
+			}
 		}
 	};
 
 	public switchSetCellPolicy(): void {
-		if (this.setCellLwwToFwwPolicySwitchOpSeqNumber === -1) {
-			if (this.isAttached()) {
-				this.userSwitchedSetCellPolicy = true;
-			} else {
-				this.setCellLwwToFwwPolicySwitchOpSeqNumber = 0;
-			}
+		if (this.fwwPolicy.state === "off") {
+			this.fwwPolicy = this.isAttached()
+				? { state: "local" }
+				: {
+						state: "on",
+						switchOpSeqNumber: 0,
+						cellLastWriteTracker: new SparseArray2D(),
+					};
 		}
 	}
 

packages/dds/matrix/src/test/fuzz.ts

@@ -53,7 +53,17 @@ interface SetCell {
 	value: MatrixItem<Value>;
 }
 
-export type Operation = InsertRows | InsertColumns | RemoveRows | RemoveColumns | SetCell;
+interface SwitchSetCellPolicy {
+	type: "switchSetCellPolicy";
+}
+
+export type Operation =
+	| InsertRows
+	| InsertColumns
+	| RemoveRows
+	| RemoveColumns
+	| SetCell
+	| SwitchSetCellPolicy;
 
 // This type gets used a lot as the state object of the suite; shorthand it here.
 type State = DDSFuzzTestState<SharedMatrixFactory>;
@@ -101,28 +111,33 @@ const reducer = combineReducers<Operation, State>({
 	set: ({ client }, { row, col, value }) => {
 		client.channel.setCell(row, col, value);
 	},
+	switchSetCellPolicy: ({ client }) => {
+		client.channel.switchSetCellPolicy();
+	},
 });
 
-interface GeneratorOptions {
-	insertRowWeight: number;
-	insertColWeight: number;
-	removeRowWeight: number;
-	removeColWeight: number;
-	setWeight: number;
-}
+type GeneratorOptions = Record<`${Operation["type"]}Weight`, number>;
 
 const defaultOptions: GeneratorOptions = {
-	insertRowWeight: 1,
-	insertColWeight: 1,
-	removeRowWeight: 1,
-	removeColWeight: 1,
-	setWeight: 20,
+	insertRowsWeight: 10,
+	insertColsWeight: 10,
+	removeRowsWeight: 10,
+	removeColsWeight: 10,
+	setWeight: 200,
+	switchSetCellPolicyWeight: 1,
 };
 
 function makeGenerator(
 	optionsParam?: Partial<GeneratorOptions>,
 ): AsyncGenerator<Operation, State> {
-	const { setWeight, insertColWeight, insertRowWeight, removeRowWeight, removeColWeight } = {
+	const {
+		setWeight,
+		insertColsWeight,
+		insertRowsWeight,
+		removeRowsWeight,
+		removeColsWeight,
+		switchSetCellPolicyWeight,
+	} = {
 		...defaultOptions,
 		...optionsParam,
 	};
@@ -178,17 +193,27 @@ function makeGenerator(
 		])(),
 	});
 
+	const switchSetCellPolicy: Generator<SwitchSetCellPolicy, State> = () => ({
+		type: "switchSetCellPolicy",
+	});
+
 	const syncGenerator = createWeightedGenerator<Operation, State>([
 		[
 			setKey,
 			setWeight,
 			(state): boolean =>
 				state.client.channel.rowCount > 0 && state.client.channel.colCount > 0,
 		],
-		[insertRows, insertRowWeight],
-		[insertCols, insertColWeight],
-		[removeRows, removeRowWeight, (state): boolean => state.client.channel.rowCount > 0],
-		[removeCols, removeColWeight, (state): boolean => state.client.channel.colCount > 0],
+		[insertRows, insertRowsWeight],
+		[insertCols, insertColsWeight],
+		[removeRows, removeRowsWeight, (state): boolean => state.client.channel.rowCount > 0],
+		[removeCols, removeColsWeight, (state): boolean => state.client.channel.colCount > 0],
+		[
+			switchSetCellPolicy,
+			switchSetCellPolicyWeight,
+			(state): boolean =>
+				state.client.channel.isSetCellConflictResolutionPolicyFWW() === false,
+		],
 	]);
 
 	return async (state) => syncGenerator(state);

packages/test/local-server-stress-tests/package.json

@@ -76,7 +76,6 @@
 		"@fluidframework/id-compressor": "workspace:~",
 		"@fluidframework/local-driver": "workspace:~",
 		"@fluidframework/map": "workspace:~",
-		"@fluidframework/matrix": "workspace:~",
 		"@fluidframework/runtime-definitions": "workspace:~",
 		"@fluidframework/runtime-utils": "workspace:~",
 		"@fluidframework/sequence": "workspace:~",
@@ -107,8 +106,7 @@
 				"^api-extractor:commonjs",
 				"@fluidframework/id-compressor#build:test",
 				"@fluidframework/sequence#build:test",
-				"@fluidframework/map#build:test",
-				"@fluidframework/matrix#build:test"
+				"@fluidframework/map#build:test"
 			]
 		}
 	},