cleanup(ygot): `render` cleanups and minor improvements for `reflect` uses #831

jayzhudev · 2023-05-06T15:37:54Z

This is a split change from the combined PR of node cache implementation (#830).

coveralls · 2023-05-15T19:08:51Z

coverage: 89.644% (-0.005%) from 89.649% when pulling 26dc738 on jayzhudev:ygot-perf into 0a6b89a on openconfig:master.

wenovus · 2023-05-22T22:52:02Z

ygot/render.go

+// CopyReserve performs normal Copy and reserves extra space for the slices.
+func (g *gnmiPath) CopyReserve(reservedSize int) *gnmiPath {


Also note that for PathElem paths it performs a shallow copy.

wenovus · 2023-05-22T23:32:06Z

ygot/struct_validation_map.go

 // structTagToLibPaths takes an input struct field as a reflect.Type, and determines
-// the set of validation library paths that it maps to. Returns the paths as a slice of
-// empty interface slices, or an error.
+// the set of validation library paths that it maps to.
 //
-// Note: the returned paths use a shallow copy of the parentPath.
-func structTagToLibPaths(f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) ([]*gnmiPath, error) {
+// Note: the mapPaths input is modified in-place.
+func structTagToLibPaths(mapPaths []*gnmiPath, f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) (int, error) {


Update comment. Where does the mapPaths input come from? What does it return? Is mapPaths modified?

wenovus · 2023-05-22T23:33:35Z

ygot/struct_validation_map.go

+// pathAnnotationToLibPaths takes the already looked-up pathAnnotation as input and determines
+// the set of validation library paths that it describes.
+//
+// Note: the mapPaths input is modified in-place.
+func pathAnnotationToLibPaths(mapPaths []*gnmiPath, pathAnnotation string, parentPath *gnmiPath) (int, error) {


Explain the other inputs and output of the function.

wenovus · 2023-05-22T23:39:54Z

ygot/render.go

+		case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32:
+			value = field.Elem().Int()
+		case reflect.Int64:
+			if args.jType == RFC7951 {
+				val := field.Elem().Int()
+				value = strconv.FormatInt(val, 10)
+			} else {
+				value = field.Elem().Int()
+			}
+		case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32:
+			value = field.Elem().Uint()
+		case reflect.Uint64:
+			if args.jType == RFC7951 {
+				val := field.Elem().Uint()
+				value = strconv.FormatUint(val, 10)
+			} else {
+				value = field.Elem().Uint()
+			}
+		case reflect.Float32:
+			value = field.Elem().Float()
+		case reflect.Float64:
+			if args.jType == RFC7951 {
+				value = fmt.Sprintf("%g", field.Elem().Float())
+			} else {
+				value = field.Elem().Float()
+			}
+		case reflect.String:
+			value = field.Elem().String()
+		case reflect.Bool:
+			value = field.Elem().Bool()
 		default:
 			value = field.Elem().Interface()
 			if args.jType == RFC7951 {
 				value = writeIETFScalarJSON(value)


This looks like it's generally useful for writeIETFScalarJSON. I think it makes sense to change the implementation of writeIETFScalarJSON to have these case statements. You can create a helper that writeIETFScalarJSON calls that takes in reflect.Value and reflect.Kind. This way we also shorten this function for readers.

wenovus · 2023-05-22T23:40:49Z

ygot/render.go

+	// When jType == RFC7951, per the IETF RFC7951 specificatioin,
+	// uint64, int64 and float64 values are represented as strings.


So if you incorporate the logic below into a helper for writeIETFScalarJSON then this can be removed.

wenovus · 2023-05-22T23:41:52Z

ygot/render.go

-			errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err))
-			continue
-		}
+		func() {


What's the reason for putting this in a function? Did you mean to extract this out into a helper?

I think this was for convenience of putting back to the sync.Pool using defer. I personally don't prefer this approach as well, and the use of sync.Pool is not giving much performance gain in this case. I'm inclined to revert the use of sync.Pool.

@wenovus Sorry for the delay. June was a very busy month on multiple tasks for me. I'll get back to this soon.

wenovus · 2023-05-22T23:57:40Z

ygot/render.go

-			if prependmods != nil && p.Len() != len(prependmods[i]) {
-				errs.Add(fmt.Errorf("number of paths and modules elements not the same: (paths: %v, modules: %v)", p, prependmods[i]))
+			if !strings.Contains(pathAnnotation, "/") {
+				parent[pathAnnotation] = value
 				continue
 			}


Ygot was not designed for very high performance situations in mind, so I think it might not be appropriate for micro-optimizations like this to be here. I'm ok with more generic optimizations like the node caching, but it's unlikely that we won't introduce performance regressions in the future since it is highly unintuitive to write code like this if the writer doesn't care about performance to the same level, and we're unlikely to enforce any stringent performance metrics.

wenovus · 2023-05-22T23:59:34Z

ygot/render.go

+
+			var kn string
+			switch keyval := keyval.(type) {
+			case string:
+				kn = keyval
+			default:
+				kn = fmt.Sprintf("%v", keyval)
+			}
+


This is another example of a micro-optimization that I think we're unlikely to enforce in this repository for the future. It makes the code more verbose but doesn't benefit most of the use cases of the repo.

If you add a clear comment explaining why this is here I'm ok with accepting it, but I want you to be aware that we might very well choose to not to abide by such micro-optimizations in the future.

wenovus · 2023-05-23T00:01:37Z

ygot/render.go

 func writeIETFScalarJSON(i interface{}) interface{} {
 	switch reflect.ValueOf(i).Kind() {
-	case reflect.Uint64, reflect.Int64, reflect.Float64:
+	case reflect.Float64:
 		return fmt.Sprintf("%v", i)
+	case reflect.Int64:
+		if val, ok := i.(int64); ok {
+			return strconv.FormatInt(int64(val), 10)
+		}
+
+		return fmt.Sprintf("%d", i)
+	case reflect.Uint64:
+		if val, ok := i.(uint64); ok {
+			return strconv.FormatUint(uint64(val), 10)
+		}
+
+		return fmt.Sprintf("%d", i)
 	}


As suggested below it would be nice if the cases here can be updated and the cases below removed in favour of this to benefit all users of this function.

wenovus · 2023-05-23T00:05:14Z

ygot/render.go

+		func() {
+			mapPaths := mapPathsPool.Get().([]*gnmiPath)
+			defer mapPathsPool.Put(mapPaths)


It would be nice if this can be refactored into its own function now that there are more optimizations and code added.

Thanks a lot for all the comments and notes. I agree that some of them seem more like trading complexity for small gains now (many are side changes added while hunting for big performance improvements for our particular use case). The use of sync.Pool here isn't justified by significant GC pressure reduction as well. I'll read through all of your comments again and I think we can probably revert the use of sync.Pool, that way this PR can be much simplified and I think it can focus on improving writeIETFScalarJSON based on your comment.

jayzhudev · 2023-07-26T00:24:34Z

Since last time we visited this PR there have been lots of changes made to the master branch. I simplified this PR as it was mentioned previously that there are micro-optimizations we may not want to include, and uses of sync.Pool we may want to remove. This PR has now become a small change that brings in some cleanups and minor improvements to the use of reflect in render.go. Thank you!

jayzhudev mentioned this pull request May 9, 2023

perf(node): caching implementation for node operations #830

Open

wenovus reviewed May 23, 2023

View reviewed changes

simplification(ygot): catch up with master and simplify the change

26dc738

jayzhudev force-pushed the ygot-perf branch from 6acb1e9 to 26dc738 Compare July 26, 2023 00:19

jayzhudev changed the title ~~perf(ygot): improve perf for render and struct_validation_map~~ cleanup(ygot): render cleanups and minor improvements for reflect uses Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(ygot): `render` cleanups and minor improvements for `reflect` uses #831

cleanup(ygot): `render` cleanups and minor improvements for `reflect` uses #831

jayzhudev commented May 6, 2023

coveralls commented May 15, 2023 •

edited

Loading

wenovus May 22, 2023

wenovus May 22, 2023

wenovus May 22, 2023

wenovus May 22, 2023

wenovus May 22, 2023

wenovus May 22, 2023

jayzhudev May 23, 2023

jayzhudev Jul 7, 2023

wenovus May 22, 2023

wenovus May 22, 2023

wenovus May 23, 2023

wenovus May 23, 2023

jayzhudev May 23, 2023

jayzhudev commented Jul 26, 2023 •

edited

Loading

		// CopyReserve performs normal Copy and reserves extra space for the slices.
		func (g gnmiPath) CopyReserve(reservedSize int) gnmiPath {

		// When jType == RFC7951, per the IETF RFC7951 specificatioin,
		// uint64, int64 and float64 values are represented as strings.

cleanup(ygot): render cleanups and minor improvements for reflect uses #831

Are you sure you want to change the base?

cleanup(ygot): render cleanups and minor improvements for reflect uses #831

Conversation

jayzhudev commented May 6, 2023

coveralls commented May 15, 2023 • 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

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

jayzhudev commented Jul 26, 2023 • edited Loading

cleanup(ygot): `render` cleanups and minor improvements for `reflect` uses #831

cleanup(ygot): `render` cleanups and minor improvements for `reflect` uses #831

coveralls commented May 15, 2023 •

edited

Loading

jayzhudev commented Jul 26, 2023 •

edited

Loading