fix deep equal check failure in CreateOrUpdateWork(), by replace the marshaler #5939

zach593 · 2024-12-11T15:41:37Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
see #5938

Which issue(s) this PR fixes:
Fixes #5938

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2024-12-11T15:57:08Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.37%. Comparing base (8313813) to head (ce1ca99).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5939   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files         666      666           
  Lines       54831    54831           
=======================================
  Hits        26524    26524           
  Misses      26590    26590           
  Partials     1717     1717

Flag	Coverage Δ
unittests	`48.37% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chaosi-zju · 2025-01-08T09:05:50Z

Hi @zach593, I am trying to use this issue to design a performance monitoring case, my case is as follows:

Record Initial Workqueue Metrics: Capture metrics before changes.
Create N Deployments and N Policies: Set up deployments and policies, then wait for propagation to member1 and member2 clusters.
Create N OverridePolicies: Implement overridepolicies to modify deployment labels for member1 cluster only. (Prior to this PR, deployments in both clusters would be updated, but after this PR, only the member1 cluster's deployments will be updated. This PR will reduce the update time for each reconciliation).
Record End Workqueue Metrics: Capture metrics after changes and calculate total reconciliation time.

Do you think there are better optimization opportunities for my case?

XiShanYongYe-Chang

LGTM, the current pr resolves the deepcopy failure issue and dose not introduce new performance risks.

About the kind/bug label. do you expect to sync the current pr to the previous release versions? If we don't need to, maybe we can modify the label.

zach593 · 2025-01-08T14:19:48Z

Do you think there are better optimization opportunities for my case?

@chaosi-zju As we discussed in zoom, for the performance optimization, this PR is not enough, I may need another 2 PRs to complete this goal. Since I need more than one PR, I created #6017 to track it, and we can have further discussion in that issue.

zach593 · 2025-01-08T15:54:40Z

About the kind/bug label. do you expect to sync the current pr to the previous release versions? If we don't need to, maybe we can modify the label.

Sure, can you help with that? I'm not familiar with commands with /

zach593 · 2025-01-08T15:58:15Z

BTW, I think we could consider use json.Marshal() instead of all of x.MarshalJSON(). Because json.Marshal() is more general, it will also call x.MarshalJSON() internally, and will also perform some other optimizations.

XiShanYongYe-Chang · 2025-01-09T02:12:50Z

Sure, can you help with that? I'm not familiar with commands with /

/remove-kind bug
/kind cleanup

chaosi-zju · 2025-01-09T02:55:44Z

As we discussed in zoom, for the performance optimization, this PR is not enough, I may need another 2 PRs to complete this goal. Since I need more than one PR, I created #6017 to track it, and we can have further discussion in that issue.

ok, thanks, I think this PR can move forward first

/lgtm

RainbowMango

/assign
I will take a look this week.

RainbowMango · 2025-01-09T13:02:08Z

pkg/util/helper/work.go

@@ -54,7 +55,7 @@ func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta meta
 		}
 	}

-	workloadJSON, err := resource.MarshalJSON()
+	workloadJSON, err := json.Marshal(resource)


Is there any difference between encoding/json.Marshal and unstructured.MarshalJSON other than the /n?
I am mostly curious about functionality and performance.

In addition, I think we need to check all of the places where using the unstructured.MarshalJSON.

PS: I don't mean to check and fix them all in this PR. Just remind me.

Tracked this on #6031, please correct me if anyone feels this is unnecessary.

Is there any difference between encoding/json.Marshal and unstructured.MarshalJSON other than the /n?
I am mostly curious about functionality and performance.

I did a benchmark test to compare different json marshal function, my test code is as:

benchmark test code

func BenchmarkMarshalJSON(b *testing.B) { resource := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "apps/v1", "kind": "Deployment", "metadata": map[string]interface{}{ "name": "test-deployment", "namespace": "default", }, "spec": map[string]interface{}{ "replicas": 3, "selector": map[string]interface{}{ "matchLabels": map[string]interface{}{ "app": "test", }, }, "template": map[string]interface{}{ "metadata": map[string]interface{}{ "labels": map[string]interface{}{ "app": "test", }, }, "spec": map[string]interface{}{ "containers": []interface{}{ map[string]interface{}{ "name": "test-container", "image": "nginx:latest", }, }, }, }, }, }, } b.Run("resource.MarshalJSON", func(b *testing.B) { for i := 0; i < b.N; i++ { _, _ = resource.MarshalJSON() } }) b.Run("json.Marshal", func(b *testing.B) { for i := 0; i < b.N; i++ { // "encoding/json" _, _ = json.Marshal(resource) } }) b.Run("jsoniter.Marshal", func(b *testing.B) { for i := 0; i < b.N; i++ { // jsoniter "github.com/json-iterator/go" _, _ = jsoniter.Marshal(resource) } }) }

the result is:

goos: windows goarch: amd64 pkg: github.com/karmada-io/karmada/pkg/controllers/ctrlutil cpu: Intel(R) Core(TM) i5-14600KF BenchmarkMarshalJSON BenchmarkMarshalJSON/resource.MarshalJSON BenchmarkMarshalJSON/resource.MarshalJSON-20 398614 2835 ns/op BenchmarkMarshalJSON/json.Marshal BenchmarkMarshalJSON/json.Marshal-20 307854 4004 ns/op BenchmarkMarshalJSON/jsoniter.Marshal BenchmarkMarshalJSON/jsoniter.Marshal-20 393528 3019 ns/op PASS

and

goos: windows goarch: amd64 pkg: github.com/karmada-io/karmada/pkg/util/helper cpu: Intel(R) Xeon(R) Gold 6278C CPU @ 2.60GHz BenchmarkMarshalJSON BenchmarkMarshalJSON/resource.MarshalJSON BenchmarkMarshalJSON/resource.MarshalJSON-4 95715 13065 ns/op BenchmarkMarshalJSON/json.Marshal BenchmarkMarshalJSON/json.Marshal-4 68590 16391 ns/op BenchmarkMarshalJSON/jsoniter.Marshal BenchmarkMarshalJSON/jsoniter.Marshal-4 79052 13281 ns/op PASS

seems previous unstructured.MarshalJSON perform better, while encoding/json.Marshal worst

resource.MarshalJSON() is specifically optimized for the unstructured.Unstructured type, potentially offering significant performance improvements when handling large numbers of Kubernetes resources.

besides, Kubernetes resources may contain fields that require special treatment during serialization, which can be handled by the custom method, for example:

Timestamp fields like creationTimestamp may need to be formatted in a particular way.

Some fields might need to be omitted entirely when they're empty, rather than being serialized as null.

Certain fields may require custom serialization rules to maintain backward compatibility with older API versions.

Resource-specific fields (like status in various resources) might need special treatment during serialization.

Is there any other option we can resolve this issue other than replace this function?

what about using json.Compact to remove redundant newlines.

Doubt it. Can you help test it?
I'm afraid the json.Compact also removes the space characters from the body, which probably makes the deep copy always return false.

I'm afraid the json.Compact also removes the space characters from the body, which probably makes the deep copy always return false.

I did the test as follows:

workloadJSON, err := resource.MarshalJSON() workloadJSON2, err := json.Marshal(resource) buffer := new(bytes.Buffer) if err = json.Compact(buffer, workloadJSON); err != nil {} workloadJSON3 := buffer.Bytes() klog.Infof("diff workloadJSON VS workloadJSON2: %+v", cmp.Diff(workloadJSON, workloadJSON2)) klog.Infof("diff workloadJSON VS workloadJSON3: %+v", cmp.Diff(workloadJSON, workloadJSON3)) klog.Infof("diff workloadJSON2 VS workloadJSON3: %+v", cmp.Diff(workloadJSON2, workloadJSON3))

the result is: workloadJSON2 and workloadJSON3 is equal, and workloadJSON is only one more \n that the other two.

https://github.com/golang/go/blob/go1.22.9/src/encoding/json/encode.go#L430

func marshalerEncoder(e *encodeState, v reflect.Value, opts encOpts) { if v.Kind() == reflect.Pointer && v.IsNil() { e.WriteString("null") return } m, ok := v.Interface().(Marshaler) if !ok { e.WriteString("null") return } b, err := m.MarshalJSON() if err == nil { e.Grow(len(b)) out := e.AvailableBuffer() out, err = appendCompact(out, b, opts.escapeHTML) e.Buffer.Write(out) } if err != nil { e.error(&MarshalerError{v.Type(), err, "MarshalJSON"}) } }

https://github.com/golang/go/blob/go1.22.9/src/encoding/json/indent.go#L39

// Compact appends to dst the JSON-encoded src with // insignificant space characters elided. func Compact(dst *bytes.Buffer, src []byte) error { dst.Grow(len(src)) b := dst.AvailableBuffer() b, err := appendCompact(b, src, false) dst.Write(b) return err }

json.Marshal() also does the same thing as json.Compact(). As I understand it, this is the only real difference between json.Marshal() and unstructured.MarshalJSON(), aside from some behaviors that might affect performance.

json.Marshal() also does the same thing as json.Compact(). As I understand it, this is the only real difference between json.Marshal() and unstructured.MarshalJSON(), aside from some behaviors that might affect performance.

You're great 👍

So, simply understand, m.MarshalJSON() is already overwrited, and json.Marshal() = m.MarshalJSON() + json.Compact

…marshaler Signed-off-by: zach593 <[email protected]>

RainbowMango

/lgtm
/approve

karmada-bot · 2025-01-15T10:18:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controllers/OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 11, 2024

karmada-bot requested review from whitewindmills and XiShanYongYe-Chang December 11, 2024 15:41

karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 11, 2024

zach593 mentioned this pull request Dec 11, 2024

deepequal check in helper.CreateOrUpdateWork() always return false, because resource.MarshalJSON() will add \n at the end of JSON #5938

Closed

XiShanYongYe-Chang mentioned this pull request Jan 8, 2025

fix deep equal check failure on objects with runtime.RawExtension. #5940

Open

XiShanYongYe-Chang reviewed Jan 8, 2025

View reviewed changes

karmada-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jan 9, 2025

karmada-bot assigned chaosi-zju Jan 9, 2025

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2025

RainbowMango reviewed Jan 9, 2025

View reviewed changes

karmada-bot assigned RainbowMango Jan 9, 2025

RainbowMango mentioned this pull request Jan 9, 2025

[Performance] Enhancements in release-1.13 #6031

Open

3 tasks

RainbowMango reviewed Jan 9, 2025

View reviewed changes

fix deep equal check failure in CreateOrUpdateWork(), by replace the …

ce1ca99

…marshaler Signed-off-by: zach593 <[email protected]>

zach593 force-pushed the work-marshaler branch from afc1a3c to ce1ca99 Compare January 10, 2025 11:53

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2025

RainbowMango added this to the v1.13 milestone Jan 15, 2025

RainbowMango approved these changes Jan 15, 2025

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

karmada-bot merged commit ce41488 into karmada-io:master Jan 15, 2025
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix deep equal check failure in CreateOrUpdateWork(), by replace the marshaler #5939

fix deep equal check failure in CreateOrUpdateWork(), by replace the marshaler #5939

zach593 commented Dec 11, 2024

codecov-commenter commented Dec 11, 2024 •

edited

Loading

chaosi-zju commented Jan 8, 2025

XiShanYongYe-Chang left a comment

zach593 commented Jan 8, 2025

zach593 commented Jan 8, 2025

zach593 commented Jan 8, 2025

XiShanYongYe-Chang commented Jan 9, 2025

chaosi-zju commented Jan 9, 2025

RainbowMango left a comment

RainbowMango Jan 9, 2025

RainbowMango Jan 9, 2025

RainbowMango Jan 9, 2025

chaosi-zju Jan 10, 2025 •

edited

Loading

chaosi-zju Jan 10, 2025

chaosi-zju Jan 13, 2025

RainbowMango Jan 13, 2025

chaosi-zju Jan 13, 2025

zach593 Jan 13, 2025

chaosi-zju Jan 13, 2025

RainbowMango left a comment

karmada-bot commented Jan 15, 2025

fix deep equal check failure in CreateOrUpdateWork(), by replace the marshaler #5939

fix deep equal check failure in CreateOrUpdateWork(), by replace the marshaler #5939

Conversation

zach593 commented Dec 11, 2024

codecov-commenter commented Dec 11, 2024 • edited Loading

Codecov Report

chaosi-zju commented Jan 8, 2025

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

zach593 commented Jan 8, 2025

zach593 commented Jan 8, 2025

zach593 commented Jan 8, 2025

XiShanYongYe-Chang commented Jan 9, 2025

chaosi-zju commented Jan 9, 2025

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaosi-zju Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jan 15, 2025

codecov-commenter commented Dec 11, 2024 •

edited

Loading

chaosi-zju Jan 10, 2025 •

edited

Loading