pkg: add slices and maps package #44981

howardjohn · 2023-05-18T17:18:12Z

This PR introduces two new packages, slices and maps.

The motivation behind this is that there are a variety of slice/map operations that are useful in Istio but not in exp/x/slices or exp/x/maps. We could just make a package only for those additions, but then we run into needing to differentiate like istioslices.Sort vs slices.Sort. So we import all the functions we need, and modify / add as needed.

Changes from upstream ones are:

Filter, Map, and Convert
Contains/Find instead of Index
Sort functions return the array for easier usage

The benefits of using these functions instead of upstream one is less boilerplate, clearer intent, and easier to optimize (for example, we do slow filtering in many places - easy to do the right thing when its just a function call away)

This PR moves a few call sites to the new package to show example usages, but it is intentionally not changing all of them since that would cause a lot of churn

zirain · 2023-05-18T22:38:41Z

there's one thing I want to ask long before:
will these package(include sets) move to istio/pkg?

howardjohn · 2023-05-18T22:42:26Z

Istio/pkg was formed when we had plan to have multi-repo and needed common code. Given we have consolidated on a mostly monorepo I don't think it provides much value separated and makes it harder to maintain so I wouldn't be in favor.

it's probably reasonable to fold the whole repo into Istio/Istio but not sure if everyone else is on board with that.

jacob-delgado · 2023-05-18T23:34:07Z

I'd be in favor of moving istio/pkg to istio. There have been times when it has needed updates or has changed and has broken istio/istio. Minimizing the chances of this would be great.

hzxuzhonghu

Most functions are a wrapper and widely used. Only function like Filter is added for special case.

Not sure whether it makes much sense

howardjohn · 2023-05-19T01:58:24Z

there is also sort returning the slice to avoid needing multiple lines, filter, etc. We can add more as needed (I actually have multiple WIP PR where I added another one to help the implementation), was just keeping this PR pretty small

…

On Thu, May 18, 2023, 6:38 PM Zhonghu Xu ***@***.***> wrote: ***@***.**** requested changes on this pull request. Most functions are a wrapper and widely used. Only function like Filter is added for special case. Not sure whether it makes much sense — Reply to this email directly, view it on GitHub <#44981 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXIZMZAM6QWEK5FX4WTXG3FJJANCNFSM6AAAAAAYGXK4MA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hzxuzhonghu · 2023-05-19T06:36:12Z

Mostly i think those new functions are not very widely used. If so, we can add to upstream.

howardjohn · 2023-05-19T14:16:38Z

Upstream has explicitly said they do not want these functions. Standard library is very conservative

…

On Thu, May 18, 2023 at 11:36 PM Zhonghu Xu ***@***.***> wrote: Mostly i think those new functions are not very widely used. If so, we can add to upstream. — Reply to this email directly, view it on GitHub <#44981 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKR7DB37JGBEFATOXTXG4IGPANCNFSM6AAAAAAYGXK4MA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

howardjohn · 2023-05-19T21:08:42Z

@hzxuzhonghu here is an example of a PR adding low-performance code because we do not have these abstractions: https://github.com/istio/istio/pull/44891/files#diff-e3b667b806175fc66cb679217641e1638639507c364df2432a60c1d4e87a9739R150-R154

This could have been slices.Filter(deletedClusters, builtClusters.Contains), saving 7 LOC, more clear, and fewer allocations (filter in place instead of new slice)

hzxuzhonghu · 2023-05-22T03:01:32Z

pkg/slices/slices.go

+}
+
+// FindFunc finds the first element matching the function, or nil if none do
+func FindFunc[E any](s []E, f func(E) bool) *E {


What if it is a []*pointer, but with an elment == nil.

In that case, E=*pointer. So **E is the response.

If we made it func FindFunc[E any](s []E, f func(E) bool) E then you would not be able to distinguish in your case.

We could make it func FindFunc[E any](s []E, f func(E) bool) (E, bool) but I think its easier to use as written here

hzxuzhonghu · 2023-05-22T03:02:07Z

pkg/slices/slices.go

+// Filter retains all elements in []E that f(E) returns true for.
+// The array is *mutated in place* and returned.
+// Used Filtered to avoid mutation
+func Filter[E any](s []E, f func(E) bool) []E {


It is kind of confusing with Filtered

I renamed it

hzxuzhonghu

Seems these wrapper are inspired by rustlang.

howardjohn · 2023-05-22T14:35:07Z

Seems these wrapper are inspired by rustlang.

Filter on a slice is common in almost every language - rust, python, java, etc

howardjohn · 2023-05-23T17:16:13Z

I have built #45058 on top of this PR, so it would be good to get direction one way or another on this. I can remove some functions if they are controversial

hzxuzhonghu · 2023-05-24T01:58:12Z

pkg/slices/slices.go

+
+// FilterInPlace retains all elements in []E that f(E) returns true for.
+// The array is *mutated in place* and returned.
+// Used Filter to avoid mutation


Suggested change

// Used Filter to avoid mutation

// Use Filter to avoid mutation

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label May 18, 2023

howardjohn requested review from a team as code owners May 18, 2023 17:18

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 18, 2023

howardjohn force-pushed the pkg/maps-slices branch from 6d0b0c8 to afc2e5d Compare May 18, 2023 17:29

howardjohn force-pushed the pkg/maps-slices branch 2 times, most recently from c5b160d to edefd1f Compare May 18, 2023 23:28

hzxuzhonghu requested changes May 19, 2023

View reviewed changes

zirain mentioned this pull request May 19, 2023

merge istio.io/pkg into istio.io/istio/pkg #44991

Closed

hzxuzhonghu reviewed May 22, 2023

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 22, 2023

howardjohn force-pushed the pkg/maps-slices branch from edefd1f to 972fc1d Compare May 23, 2023 15:18

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label May 23, 2023

howardjohn force-pushed the pkg/maps-slices branch from 972fc1d to 71dc22f Compare May 23, 2023 15:27

howardjohn requested a review from hzxuzhonghu May 23, 2023 15:28

howardjohn mentioned this pull request May 23, 2023

GAMMA: remove requirement for ReferenceGrant for cross namespace backends #45058

Merged

howardjohn force-pushed the pkg/maps-slices branch from 71dc22f to 2edd69b Compare May 23, 2023 17:18

hzxuzhonghu reviewed May 24, 2023

View reviewed changes

hzxuzhonghu approved these changes May 24, 2023

View reviewed changes

hanxiaop approved these changes May 24, 2023

View reviewed changes

ericvn approved these changes May 24, 2023

View reviewed changes

Initial introduction of slices package

126ac64

howardjohn force-pushed the pkg/maps-slices branch from 2edd69b to 126ac64 Compare May 24, 2023 16:13

istio-testing merged commit 9a9d1c8 into istio:master May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: add slices and maps package #44981

pkg: add slices and maps package #44981

howardjohn commented May 18, 2023

zirain commented May 18, 2023

howardjohn commented May 18, 2023

jacob-delgado commented May 18, 2023

hzxuzhonghu left a comment

howardjohn commented May 19, 2023 via email

hzxuzhonghu commented May 19, 2023

howardjohn commented May 19, 2023 via email

howardjohn commented May 19, 2023

hzxuzhonghu May 22, 2023

howardjohn May 23, 2023

hzxuzhonghu May 22, 2023

howardjohn May 23, 2023

hzxuzhonghu left a comment

howardjohn commented May 22, 2023

howardjohn commented May 23, 2023

hzxuzhonghu May 24, 2023

	// Used Filter to avoid mutation
	// Use Filter to avoid mutation

pkg: add slices and maps package #44981

pkg: add slices and maps package #44981

Conversation

howardjohn commented May 18, 2023

zirain commented May 18, 2023

howardjohn commented May 18, 2023

jacob-delgado commented May 18, 2023

hzxuzhonghu left a comment

Choose a reason for hiding this comment

howardjohn commented May 19, 2023 via email

hzxuzhonghu commented May 19, 2023

howardjohn commented May 19, 2023 via email

howardjohn commented May 19, 2023

hzxuzhonghu May 22, 2023

Choose a reason for hiding this comment

howardjohn May 23, 2023

Choose a reason for hiding this comment

hzxuzhonghu May 22, 2023

Choose a reason for hiding this comment

howardjohn May 23, 2023

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

howardjohn commented May 22, 2023

howardjohn commented May 23, 2023

hzxuzhonghu May 24, 2023

Choose a reason for hiding this comment