Xiao-zhen-Liu commented on code in PR #6927:
URL: https://github.com/apache/texera/pull/6927#discussion_r3696927267
##########
frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts:
##########
@@ -491,11 +493,22 @@ export class JointUIService {
* @param isOperatorValid
*/
public changeOperatorColor(jointPaper: joint.dia.Paper, operatorID: string,
isOperatorValid: boolean): void {
- if (isOperatorValid) {
- jointPaper.getModelById(operatorID).attr("rect.body/stroke", "#CFCFCF");
- } else {
- jointPaper.getModelById(operatorID).attr("rect.body/stroke", "red");
+ this.paintOperatorBorder(jointPaper, operatorID, isOperatorValid ?
"#CFCFCF" : "red");
+ }
+
+ /**
+ * Sets the operator's border stroke, skipping the write when the border is
+ * already that color. On operator add, the operator-add restore and the
+ * validation pass both request a border color for the same operator;
guarding
+ * the write here makes the redundant repaint a no-op (see #5726) and
likewise
Review Comment:
The redundant repaint was already a no-op. Backbone's `Model.set` compares
with `_.isEqual` and fires no change event for a same-value write, so
`attr("rect.body/stroke", sameColor)` never reached the renderer.
What the guard really skips is jointjs cloning and deep-comparing the attrs
tree, which is most of the cost of a redundant `attr()` call (~46us on our
element, down to ~0.6us). That's the actual reason to keep it on
`changeOperatorColor`, so it's worth saying that here instead.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -911,6 +912,18 @@ describe("WorkflowEditorComponent", () => {
expect(getStroke(mockScanPredicate.operatorID)).toBe("green");
});
+ it("paints the execution-state stroke (orange) for a valid operator with
a cached Running status", () => {
Review Comment:
This is the part I'd like to keep. It's real coverage of the #3614 reload
behaviour for a cached state other than Completed, independent of the rest of
the PR.
One change: point the comment at #3614 and drop the #5726 reference, since
I'm re-scoping that issue.
##########
frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts:
##########
@@ -460,10 +460,12 @@ export class JointUIService {
}
jointPaper.getModelById(operatorID).attr({
[`.${operatorStateClass}`]: { text: operatorState.toString(), fill:
fillColor },
- "rect.body": { stroke: fillColor },
[`.${operatorPortMetricsClass}`]: { fill: fillColor },
[`.${operatorWorkerCountClass}`]: { fill: fillColor },
});
+ // Border stroke goes through the guarded setter so a repaint with the
color
+ // it already has is a no-op (see #5726).
+ this.paintOperatorBorder(jointPaper, operatorID, fillColor);
Review Comment:
This is the one spot where the guard can't pay off. The bundled
`attr({...})` above already deep-merges and compares the whole attrs tree, so
pulling the stroke out of it adds a second write rather than avoiding one.
Measured on the pinned versions, a real state change goes from ~45us to
~97us, and from one change event to two. Unchanged ticks are a wash.
Putting `"rect.body": { stroke: fillColor }` back in the bundled payload and
dropping this call fixes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
