argoproj / gitops-engine

Democratizing GitOps
https://pkg.go.dev/github.com/argoproj/gitops-engine?tab=subdirectories
Apache License 2.0
1.7k stars 265 forks source link

chore: avoid unnecessary json unmarshal #627

Closed crenshaw-dev closed 2 months ago

crenshaw-dev commented 2 months ago

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)
    }
}
sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

crenshaw-dev commented 2 months ago

Argo CD tests passed, merging. Thanks, Leo!