-
Notifications
You must be signed in to change notification settings - Fork 29
Fix stuck saving when more than ~1500 buckets were labeled at once #8409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6607d49
a8d1556
c3ef03b
af5c69c
ccc3a59
3184521
9e89b9b
4c73439
6c9113c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1199,7 +1199,7 @@ function* handleMeshVisibilityChange(action: UpdateMeshVisibilityAction): Saga<v | |
segmentMeshController.setMeshVisibility(id, visibility, layerName, additionalCoordinates); | ||
} | ||
|
||
export function* handleAdditionalCoordinateUpdate(): Saga<void> { | ||
export function* handleAdditionalCoordinateUpdate(): Saga<never> { | ||
// We want to prevent iterating through all additional coordinates to adjust the mesh visibility, so we store the | ||
// previous additional coordinates in this method. Thus we have to catch SET_ADDITIONAL_COORDINATES actions in a | ||
// while-true loop and register this saga in the root saga instead of calling from the mesh saga. | ||
|
@@ -1212,11 +1212,13 @@ export function* handleAdditionalCoordinateUpdate(): Saga<void> { | |
const action = (yield* take(["SET_ADDITIONAL_COORDINATES"]) as any) as FlycamAction; | ||
// Satisfy TS | ||
if (action.type !== "SET_ADDITIONAL_COORDINATES") { | ||
throw new Error("Unexpected action type"); | ||
// Don't throw as this would interfere with the never return type | ||
console.error("Unexpected action.type. Ignoring SET_ADDITIONAL_COORDINATES action..."); | ||
continue; | ||
} | ||
const meshRecords = segmentMeshController.meshesGroupsPerSegmentId; | ||
|
||
if (action.values == null || action.values.length === 0) break; | ||
if (action.values == null || action.values.length === 0) continue; | ||
Comment on lines
+1217
to
+1221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this ok to change? The previously thrown error is now swallowed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the handling is only done for TS, as far as I understand. I can output a warning to console, though. |
||
const newAdditionalCoordKey = getAdditionalCoordinatesAsString(action.values); | ||
|
||
for (const additionalCoordinates of [action.values, previousAdditionalCoordinates]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import Toast from "libs/toast"; | |
import { sleep } from "libs/utils"; | ||
import window, { alert, document, location } from "libs/window"; | ||
import _ from "lodash"; | ||
import memoizeOne from "memoize-one"; | ||
import messages from "messages"; | ||
import { ControlModeEnum } from "oxalis/constants"; | ||
import { getMagInfo } from "oxalis/model/accessors/dataset_accessor"; | ||
|
@@ -57,7 +58,7 @@ import { takeEveryWithBatchActionSupport } from "./saga_helpers"; | |
|
||
const ONE_YEAR_MS = 365 * 24 * 3600 * 1000; | ||
|
||
export function* pushSaveQueueAsync(): Saga<void> { | ||
export function* pushSaveQueueAsync(): Saga<never> { | ||
yield* call(ensureWkReady); | ||
|
||
yield* put(setLastSaveTimestampAction()); | ||
|
@@ -87,7 +88,7 @@ export function* pushSaveQueueAsync(): Saga<void> { | |
}); | ||
yield* put(setSaveBusyAction(true)); | ||
|
||
// Send (parts) of the save queue to the server. | ||
// Send (parts of) the save queue to the server. | ||
// There are two main cases: | ||
// 1) forcePush is true | ||
// The user explicitly requested to save an annotation. | ||
|
@@ -100,7 +101,7 @@ export function* pushSaveQueueAsync(): Saga<void> { | |
// The auto-save interval was reached at time T. The following code | ||
// will determine how many items are in the save queue at this time T. | ||
// Exactly that many items will be sent to the server. | ||
// New items that might be added to the save queue during saving, will | ||
// New items that might be added to the save queue during saving, will be | ||
// ignored (they will be picked up in the next iteration of this loop). | ||
// Otherwise, the risk of a high number of save-requests (see case 1) | ||
// would be present here, too (note the risk would be greater, because the | ||
|
@@ -118,7 +119,6 @@ export function* pushSaveQueueAsync(): Saga<void> { | |
break; | ||
} | ||
} | ||
|
||
yield* put(setSaveBusyAction(false)); | ||
} | ||
} | ||
|
@@ -183,7 +183,6 @@ export function* sendSaveRequestToServer(): Saga<number> { | |
const startTime = Date.now(); | ||
yield* call( | ||
sendRequestWithToken, | ||
|
||
`${tracingStoreUrl}/tracings/annotation/${annotationId}/update?token=`, | ||
{ | ||
method: "POST", | ||
|
@@ -281,12 +280,16 @@ export function* sendSaveRequestToServer(): Saga<number> { | |
} | ||
|
||
function* markBucketsAsNotDirty(saveQueue: Array<SaveQueueEntry>) { | ||
const getLayerAndMagInfoForTracingId = memoizeOne((tracingId: string) => { | ||
const segmentationLayer = Model.getSegmentationTracingLayer(tracingId); | ||
const segmentationMagInfo = getMagInfo(segmentationLayer.mags); | ||
return [segmentationLayer, segmentationMagInfo] as const; | ||
}); | ||
Comment on lines
282
to
+287
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added memoization to optimize bucket processing Great performance optimization! This directly addresses the PR objective by reducing redundant calculations in the The memoization ensures that for a given |
||
for (const saveEntry of saveQueue) { | ||
for (const updateAction of saveEntry.actions) { | ||
if (updateAction.name === "updateBucket") { | ||
const { actionTracingId: tracingId } = updateAction.value; | ||
const segmentationLayer = Model.getSegmentationTracingLayer(tracingId); | ||
const segmentationMagInfo = yield* call(getMagInfo, segmentationLayer.mags); | ||
const [segmentationLayer, segmentationMagInfo] = getLayerAndMagInfoForTracingId(tracingId); | ||
Comment on lines
-288
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fix, correct? Strange that non-nested yield calls can lead to a stack overflow :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, apparently, the effects are processed tail recursively by redux which leads to this problem. |
||
|
||
const { position, mag, additionalCoordinates } = updateAction.value; | ||
const magIndex = segmentationMagInfo.getIndexByMag(mag); | ||
|
@@ -377,7 +380,7 @@ export function* saveTracingAsync(): Saga<void> { | |
|
||
export function* setupSavingForTracingType( | ||
initializeAction: InitializeSkeletonTracingAction | InitializeVolumeTracingAction, | ||
): Saga<void> { | ||
): Saga<never> { | ||
/* | ||
Listen to changes to the annotation and derive UpdateActions from the | ||
old and new state. | ||
|
@@ -450,7 +453,7 @@ const VERSION_POLL_INTERVAL_COLLAB = 10 * 1000; | |
const VERSION_POLL_INTERVAL_READ_ONLY = 60 * 1000; | ||
const VERSION_POLL_INTERVAL_SINGLE_EDITOR = 30 * 1000; | ||
|
||
function* watchForSaveConflicts() { | ||
function* watchForSaveConflicts(): Saga<never> { | ||
function* checkForNewVersion() { | ||
const allowSave = yield* select( | ||
(state) => state.tracing.restrictions.allowSave && state.tracing.restrictions.allowUpdate, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed break to continue when values are empty
This is a crucial fix that likely addresses the core issue. Previously, the saga would exit with
break
whenaction.values
was null or empty, which would terminate the saga permanently. Changing tocontinue
ensures the saga keeps listening for subsequent actions, maintaining its long-running nature.