Skip to content
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

chore: avoid unnecessary json unmarshal #627

Merged

Conversation

crenshaw-dev
Copy link
Member

As far as I can tell, the only purpose these lines serve is to confirm that predictedLiveBytes is valid JSON. But at this point in the code, I don't think we have any reason to believe that the JSON is not valid. We're relying on solid libraries to produce that JSON. Even if the JSON were somehow invalid, I'm not sure there's any value in validating it here versus where it's eventually used.

Here's the benchmark output:

before: Benchmark_threeWayMergePatch-16            21823             53131 ns/op           41018 B/op        861 allocs/op
after:  Benchmark_threeWayMergePatch-16            22926             52559 ns/op           39072 B/op        810 allocs/op

It's a small but not insignificant memory and compute win. I think the benefits increase as the JSON size increases.

And the benchmark code:

func Benchmark_threeWayMergePatch(b *testing.B) {
	orig := []byte(`
apiVersion: v1
kind: Service
metadata:
  name: my-service
  namespace: default
`)
	config := []byte(`
apiVersion: v1
kind: Service
metadata:
  name: my-service
  namespace: default
`)
	live := []byte(`
apiVersion: v1
kind: Service
metadata:
  name: my-service
  namespace: default
`)

	origUnstructured := unstructured.Unstructured{}
	err := yaml.Unmarshal(orig, &origUnstructured)
	require.NoError(b, err)
	configUnstructured := unstructured.Unstructured{}
	err = yaml.Unmarshal(config, &configUnstructured)
	require.NoError(b, err)
	liveUnstructured := unstructured.Unstructured{}
	err = yaml.Unmarshal(live, &liveUnstructured)
	require.NoError(b, err)

	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_, err := ThreeWayDiff(&origUnstructured, &configUnstructured, &liveUnstructured)
		require.NoError(b, err)
	}
}

Copy link

sonarcloud bot commented Sep 16, 2024

Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. While I don't understand how this could cause issues, please make sure all tests are passing when updating Argo CD.

@crenshaw-dev
Copy link
Member Author

Argo CD tests passed, merging. Thanks, Leo!

@crenshaw-dev crenshaw-dev merged commit df9b446 into argoproj:master Sep 16, 2024
3 checks passed
@crenshaw-dev crenshaw-dev deleted the avoid-unnecessary-unmarshal branch September 16, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants