Envoyfilter matching performance optimization by hzxuzhonghu · Pull Request #19786 · istio/istio

hzxuzhonghu · 2019-12-25T09:57:58Z

move the config patch's proxy match to the top level , this is to reduce the calling of proxyMatch times,
merging envoyfilterWrappers in getting procedure from push context
The performance is improved.

Before:

$ go test  -bench=. -benchtime 30s
goos: linux
goarch: amd64
pkg: istio.io/istio/pilot/pkg/networking/core/v1alpha3/envoyfilter
BenchmarkTelemetryV2Filters-4   	  176700	    208609 ns/op

With this pr

$ go test  -bench=. -benchtime 30s
goos: linux
goarch: amd64
pkg: istio.io/istio/pilot/pkg/networking/core/v1alpha3/envoyfilter
BenchmarkTelemetryV2Filters-4   	  415954	     87968 ns/op

pilot/pkg/networking/core/v1alpha3/envoyfilter/cluster_patch_test.go

howardjohn · 2019-12-26T17:46:19Z

pilot/pkg/networking/core/v1alpha3/envoyfilter/rc_patch_test.go

 			},
 			{
-				Name:    "new-vhost",
-				Domains: []string{"domain:80"},


why remove?

The "new-vhost" is a new added VirtualHost, we should not allow mutating a user specified patch.

howardjohn · 2019-12-26T17:46:56Z

pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch_test.go

 	var got interface{}
 	b.ResetTimer()
 	for n := 0; n < b.N; n++ {
+		copied := proto.Clone(l)


why did you have to add this? This is suspicious, are we modifying the EnvoyFitlter somewhere? I don't think we should be doing that, caused a lot of problems doing a similar thing with VS

This is a bug fix, as i tested, i found that the running time is grown as the times increase. It is because, the listener length is growing each time.

This can be seen as the memory leaks

I am pretty sure this is a bug in listener patch code. We shouldn't be modifying the listeners we pass in. Otherwise we will run into issues. For example, if we have some filter as constant and we modify that, its going to be very bad

Or maybe its not an issue... what do you think?

howardjohn · 2019-12-26T17:47:33Z

pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch_test.go

@@ -724,34 +736,32 @@ func TestApplyListenerPatches(t *testing.T) {
 // This benchmark measures the performance of Telemetry V2 EnvoyFilter patches. The intent here is to
 // measure overhead of using EnvoyFilters rather than native code.
 func BenchmarkTelemetryV2Filters(b *testing.B) {


Why is this ~3x faster now? The proxy IS a match, so shouldn't there be no improvement?

Actually the most important part is i moved the proxyMatch to the PushContext.EnvoyFilters, which is actually called once per proxy. But currently, the times is correlated to the number of listeners, patches, filterchains, networkfilters, etc.

howardjohn · 2019-12-26T17:59:56Z

I do see CPU usage go from 600m to 500m with this patch on a cluster

howardjohn · 2019-12-26T19:16:49Z

howardjohn · 2019-12-26T19:19:41Z

lgtm if we can explain why its now faster, and fix the extra return in the test (unless there is a reason for it?). Thanks!

hzxuzhonghu · 2019-12-27T01:48:55Z

Thanks for the e2e performance test. And the perfomrance improvement is related to Reducing the proxyMatch calling times a lot

howardjohn · 2019-12-27T03:20:36Z

I don't think proxy match is that cpu intensive though? I would be interested to see profiles before and after this change. I can run it myself though

howardjohn · 2019-12-27T03:50:59Z

Ah so the main performance benefit is actually merging all the patches. It makes this get called only one time:

istio/pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch.go

Line 311 in 0a5be9d

if err := ptypes.UnmarshalAny(filter.GetTypedConfig(), hcm); err != nil {

The crash was introduced by istio#19786

The crash was introduced by #19786

hzxuzhonghu added 6 commits December 25, 2019 16:12

Move proxy version match against envoyfilter to top level

2209976

fix ut

51bd1e0

fix ut

ef4b196

remove unused proxy

2bb0242

fix ut

664d014

fix benchmark

a645a34

hzxuzhonghu requested a review from a team as a code owner December 25, 2019 09:57

istio-policy-bot added area/networking area/perf and scalability labels Dec 25, 2019

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 25, 2019

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 25, 2019

hzxuzhonghu added 2 commits December 25, 2019 18:18

fix lint

1e01353

fix lint

b232ec4

howardjohn reviewed Dec 26, 2019

View reviewed changes

remove the return

e5edfe2

howardjohn approved these changes Dec 27, 2019

View reviewed changes

istio-testing merged commit a327090 into istio:master Dec 27, 2019

hzxuzhonghu deleted the envoyfilter-proxy-match branch December 27, 2019 06:58

howardjohn mentioned this pull request Dec 27, 2019

Telemetry v2 EnvoyFilter leads to 160% increase in Pilot CPU #18824

Closed

nmittler added a commit to nmittler/istio that referenced this pull request Dec 27, 2019

Fixing crash introduced with recent commit

a352e5f

The crash was introduced by istio#19786

nmittler mentioned this pull request Dec 27, 2019

Fixing crash introduced with recent commit #19823

Merged

istio-testing pushed a commit that referenced this pull request Dec 28, 2019

Fixing crash introduced with recent commit (#19823)

61414a6

The crash was introduced by #19786

Conversation

hzxuzhonghu commented Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Dec 26, 2019

Uh oh!

howardjohn commented Dec 26, 2019

Uh oh!

howardjohn commented Dec 26, 2019

Uh oh!

hzxuzhonghu commented Dec 27, 2019

Uh oh!

howardjohn commented Dec 27, 2019

Uh oh!

howardjohn commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hzxuzhonghu commented Dec 25, 2019 •

edited

Loading