Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Use transformer to avoid making unnecessary event copy in delivery transformer #1514

Merged
merged 5 commits into from
Jul 28, 2020

Conversation

ian-mi
Copy link
Contributor

@ian-mi ian-mi commented Jul 24, 2020

When sending to a retry topic it is still necessary to clone EventContext since the retry client
does not yet use message bindings.

Fixes #1511

Benchmark results show significant improvement for large payload sizes since this avoids buffering payload data:

DeliveryNoReply/0_bytes-16 9.68µs ± 1% 9.26µs ± 2% -4.28% (p=0.000 n=9+10)
DeliveryNoReply/1000_bytes-16 10.4µs ± 2% 9.5µs ± 1% -8.42% (p=0.000 n=9+10)
DeliveryNoReply/1000000_bytes-16 294µs ± 2% 146µs ± 0% -50.28% (p=0.000 n=10+9)
DeliveryNoReplyFakeClient/0_bytes-16 2.63µs ± 1% 2.45µs ± 1% -6.70% (p=0.000 n=8+10)
DeliveryNoReplyFakeClient/1000_bytes-16 2.81µs ± 3% 2.47µs ± 1% -12.14% (p=0.000 n=10+10)
DeliveryNoReplyFakeClient/100000_bytes-16 14.9µs ± 1% 2.5µs ± 2% -83.51% (p=0.000 n=10+9)

@ian-mi ian-mi requested a review from yolocs July 24, 2020 21:57
@google-cla google-cla bot added the cla: yes (override cla status due to multiple authors bug) label Jul 24, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ian-mi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@yolocs yolocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up so quick. Left a nit.

@ian-mi
Copy link
Contributor Author

ian-mi commented Jul 27, 2020

Event messages are not immutable so I had to introduce an immutable version to avoid the event copy. This happened to expose a bug in the HTTP message implementation which I fixed upstream in cloudevents/sdk-go#557.

@ian-mi ian-mi requested a review from yolocs July 27, 2020 18:58
@ian-mi
Copy link
Contributor Author

ian-mi commented Jul 27, 2020

/retest

Copy link
Member

@yolocs yolocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Left a question

@ian-mi
Copy link
Contributor Author

ian-mi commented Jul 27, 2020

/retest

@ian-mi ian-mi requested a review from yolocs July 27, 2020 23:17
Copy link
Member

@yolocs yolocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/broker/eventutil/hops.go 85.0% 84.2% -0.8
pkg/broker/eventutil/immutable_event_message.go Do not exist 100.0%
pkg/broker/handler/processors/deliver/processor.go 79.5% 80.0% 0.5

@yolocs
Copy link
Member

yolocs commented Jul 28, 2020

/retest

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-google-knative-gcp-integration-tests 2020-07-27 23:52:44.559 +0000 UTC 1/3
pull-google-knative-gcp-wi-tests 2020-07-28 16:12:44.596 +0000 UTC
2020-07-28 16:52:14.397 +0000 UTC
2/3

Automatically retrying due to test flakiness...
/test pull-google-knative-gcp-wi-tests

@knative-prow-robot knative-prow-robot merged commit 1790a2d into google:master Jul 28, 2020
@ian-mi ian-mi deleted the deliver-transform branch July 28, 2020 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delivery processor event.Clone consumes too much unnecessary memroy
5 participants