ygot.TogNMINotifications generated proto message has underlying memory sharing that is not documented

[shichuzhu]

https://github.com/openconfig/ygot/blob/6f722a0cce2a47949294afa0c3f23b080d51e501/ygot/render.go#L329

For demo purpose

func demo(root ygot.GoStruct) {
    ns, _ := ygot.TogNMINotifications(root, 0, ygot.GNMINotificationsConfig{
        UsePathElem: true,
    })
    updates := ns[0].GetUpdate()
    fmt.Println(updates[1].GetPath().GetElem()[0].Name) // prints "interfaces"
    updates[0].GetPath().GetElem()[0].Name = "somethingElse"
    fmt.Println(updates[1].GetPath().GetElem()[0].Name) // it now prints "somethingElse" !
}

It looks like in the generated Notification proto messages, paths of different Updates may map to the same memory location. Yet this 'data sharing' is not mentioned in the GNMINotificationsConfig doc string.

If this is intended behavior for space optimization, consider adding a comment like

Within the generated notifications there could be data sharing for space optimization. Make a deep copy if one plans to modify the generated message

@wenovus @robshakir

[wenovus]

Thanks Stuart for reporting this issue and the suggested comment. I've added the documentation fix at https://github.com/openconfig/ygot/pull/736.

@robshakir I think the right fix is that by default, users don't have to call Clone, and that we provide an option within GNMINotificationsConfig to allow this optimization to keep the behaviour. If you agree either Stuart or I can change the default behaviour and add the flag.

---

If you're curious the reason is because we're shallow-copying the PathElems: https://github.com/openconfig/ygot/blob/1d36b4ad76e5cfb41a8b67c8c37ab725575afde4/ygot/render.go#L154-L158

But there doesn't look like a way to fix this without incurring a performance penalty:

!?master ~/gocode/src/github.com/openconfig/lsdbparse> go test -bench=. -cpuprofile cpu.out
goos: linux
goarch: amd64
pkg: github.com/openconfig/lsdbparse
BenchmarkRenderLSP/simple_example/usePathElem=false-12             35715             32565 ns/op
BenchmarkRenderLSP/larger_example/usePathElem=false-12              6960            209299 ns/op
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=false-12     54831             21176 ns/op
BenchmarkRenderLSP/simple_example/usePathElem=true-12              24684             48815 ns/op ***
BenchmarkRenderLSP/larger_example/usePathElem=true-12               5043            230933 ns/op ***
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=true-12      33808             34843 ns/op ***

diff --git a/ygot/render.go b/ygot/render.go
index 06b57bf..2bdb66d 100644
--- a/ygot/render.go
+++ b/ygot/render.go
@@ -153,7 +153,16 @@ func (g *gnmiPath) Copy() *gnmiPath {
        }

        n.pathElemPath = make([]*gnmipb.PathElem, len(g.pathElemPath))
-       copy(n.pathElemPath, g.pathElemPath)
+       for i, elem := range g.pathElemPath {
+               keys := map[string]string{}
+               for k, v := range elem.Key {
+                       keys[k] = v
+               }
+               n.pathElemPath[i] = &gnmipb.PathElem{
+                       Name: elem.Name,
+                       Key:  keys,
+               }
+       }
        return n
 }

goos: linux
goarch: amd64
pkg: github.com/openconfig/lsdbparse
BenchmarkRenderLSP/simple_example/usePathElem=false-12             39054             33514 ns/op
BenchmarkRenderLSP/larger_example/usePathElem=false-12              5790            191188 ns/op
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=false-12     57350             20845 ns/op
BenchmarkRenderLSP/simple_example/usePathElem=true-12              15856             75378 ns/op ***
BenchmarkRenderLSP/larger_example/usePathElem=true-12               2773            389370 ns/op ***
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=true-12      26888             44739 ns/op ***

diff --git a/ygot/render.go b/ygot/render.go
index 06b57bf..d3dbc46 100644
--- a/ygot/render.go
+++ b/ygot/render.go
@@ -153,7 +153,9 @@ func (g *gnmiPath) Copy() *gnmiPath {
        }

        n.pathElemPath = make([]*gnmipb.PathElem, len(g.pathElemPath))
-       copy(n.pathElemPath, g.pathElemPath)
+       for i, elem := range g.pathElemPath {
+               n.pathElemPath[i] = proto.Clone(elem).(*gnmipb.PathElem)
+       }
        return n
 }

!?master ~/gocode/src/github.com/openconfig/lsdbparse> go test -bench=. -cpuprofile cpu.out
goos: linux
goarch: amd64
pkg: github.com/openconfig/lsdbparse
BenchmarkRenderLSP/simple_example/usePathElem=false-12             36195             40345 ns/op
BenchmarkRenderLSP/larger_example/usePathElem=false-12              5386            206761 ns/op
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=false-12     60015             20391 ns/op
BenchmarkRenderLSP/simple_example/usePathElem=true-12              12013             99138 ns/op ***
BenchmarkRenderLSP/larger_example/usePathElem=true-12               2024            546587 ns/op ***
BenchmarkRenderLSP/simple_-_pathelem_path/usePathElem=true-12      22569             57197 ns/op ***