feat(translator): use upstreamMap instead of for loop in upstreams array

[weixiao-huang]

Description

This PR use upstreamMap instead of for loop of array and fixes solo-io#7973

Context

see solo-io#7973

Checklist:

• I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
• If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
• I followed guidelines laid out in the Gloo Edge contribution guide
• I opened a draft PR or added the work in progress label if my PR is not ready for review
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves solo-io#7973

[solo-build-bot]

Waiting for approval from someone in the solo-io org to start testing.

[solo-changelog-bot]

Issues linked to changelog:
solo-io#7973

[sam-heilbron]

@weixiao-huang thanks for the contribution! I like the idea here, and agree that there are some improvements we can introduce to the UpstreamList.Find functionality which currently iterates over a list instead of a quick lookup in a map.

We have an ongoing issue solo-io/solo-kit#521 to track improving the Find performance not just for Upstreams, but all types. I would rather introduce it collectively than just for a single type.

How about we move forward with the following plan:

1. If you can document the performance gains from this PR alone, you add a comment on this PR outlining the improvement
2. I will migrate the issue I linked into the Gloo repository so there is more awareness around it
3. We close out this PR, and link it in the Gloo issue. Then we use the performance measurements as a way to try to prioritize the value of this work.

How does that sound to you?

[jackstine]

1. For scale could you also provide how many Upstreams are you running that is causing issues with performance?
2. And how did you determine that this was the performance bottleneck for you?

This will help us understand the significance of this work as well, as @sam-heilbron has mentioned.

You mentioned tens of thousands in the issue, so I am left to believe this is your scale?

[weixiao-huang]

Sorry for my delay, I'd like to ask whether I should move this PR to solo-kit or just remain it in gloo? Another question is that which performance gains should I add? Is there a template?

[sam-heilbron]

Sorry for my delay, I'd like to ask whether I should move this PR to solo-kit or just remain it in gloo? Another question is that which performance gains should I add? Is there a template?

We don't have a template unfortunately. Any findings you have can be documented here. For example "i applied X upstreams and Y virtual services, and translation took N seconds before this change and M seconds after this change". if you don't have those numbers that is ok too. I figured any proof of impact would help us argue for prioritizing this work on the Engineering team here.

Once you add those details, or just respond saying you don't have them, I will do the following:

1. Create an issue in Gloo to track this feature request, and link this PR
2. Close this PR

[sam-heilbron]

@weixiao-huang Are you able to provide any details about the scale you were running and the improvements that you measured? If not, that's ok, I just wanted to make sure we had that context if it was available. Since this is a change that we can do more broadly in the product, I would like to close this PR and I will raise the issue internally. Will that work for you?

[weixiao-huang]

Sorry for my delay, I'll check it and provide details before 31st May.

[weixiao-huang]

Sorry for my delay, I'll check it and provide details before 31st May.

I'm sorry but I'm busy in these days, I'll provide it before 12th June.

[weixiao-huang]

It seems not easy to reproduce and get a benchmark. Is there any progress for solo-io/solo-kit#521 ? I've found 404 for https://github.com/solo-io/solo-projects/pull/4077 in this issue. Could I try to add a PR for changing for loop to map more generic in solo-kit?

[sam-heilbron]

It seems not easy to reproduce and get a benchmark. Is there any progress for solo-io/solo-kit#521 ? I've found 404 for solo-io/solo-projects#4077 in this issue. Could I try to add a PR for changing for loop to map more generic in solo-kit?

I've created solo-io#8988 to track this request in a more public space (this repository) so that others who have feedback on the urgency of it, or ideas for solving it can participate.

Given that this PR is a good start to a solution, but does not solve the problem holistically, I would vote that we close it. If you are interested in trying to solve this issue as written in solo-kit to provide this enhancement, that would be amazing!