server: Refactor stream observer #95

sschepens · 2019-03-12T17:05:54Z

The objective of this PR is to reduce the complexity and overhead when not operating in ADS mode.
When working with separate XDS streams the StreamObserver logic can be much simpler and it does not need to store maps, thus reducing the memory overhead.
This also includes a change in Watch and StreamObserver to use Atomic*FieldUpdaters instead of Atomic* which reduces the memory overhead per instance.

I realize this introduces a bit of indirection and probably code complexity, but this should be a win when not operating in ADS mode and having thousands of streams.

jakubdyszkiewicz

Nice! Do you have any numbers how much did it improve performance of your control plane?

jakubdyszkiewicz · 2019-03-12T18:33:29Z

server/src/main/java/io/envoyproxy/controlplane/server/AdsDiscoveryRequestStreamObserver.java

+      }
+    } else if (requestTypeUrl.isEmpty()) {
+      requestTypeUrl = defaultTypeUrl;
+    }


I think you can simplify it. For the defaultTypeUrl you always pass ANY_TYPE_URL. You can delete the defaultTypeUrl from the constructor, pass the ANY_TYPE_URL to DiscoveryRequestStreamObserver.
Then you can delete if (defaultTypeUrl.equals(ANY_TYPE_URL)) { if and also } else if (requestTypeUrl.isEmpty()) { branch

that is correct, good catch 👍

server/src/main/java/io/envoyproxy/controlplane/server/XdsDiscoveryRequestStreamObserver.java

sschepens · 2019-03-12T20:13:20Z

Haven't run any masurements yet, will do.
I don't know if the perf improvements will be very noticeable, but the memory savings should be considerable. We currently have 3 ConcurrentHashMaps per stream a quick inspection reveals that it has a lot of fields and surely has a lot of memory overhead vs 3 simple reference fields.
Atomic field updaters should also give some considerable amount of savings.

Gonna run some measurements to see exactly how much memory we would be saving.

codecov-io · 2019-03-13T02:16:28Z

Codecov Report

Merging #95 into master will decrease coverage by 0.3%.
The diff coverage is 90.9%.

@@             Coverage Diff             @@
##             master     #95      +/-   ##
===========================================
- Coverage      89.8%   89.5%   -0.31%     
- Complexity      149     193      +44     
===========================================
  Files            19      22       +3     
  Lines           569     600      +31     
  Branches         53      48       -5     
===========================================
+ Hits            511     537      +26     
- Misses           44      48       +4     
- Partials         14      15       +1

Impacted Files	Coverage Δ	Complexity Δ
...nvoyproxy/controlplane/server/DiscoveryServer.java	`100% <100%> (+5.18%)`	`15 <0> (+1)`	⬆️
...lane/server/XdsDiscoveryRequestStreamObserver.java	`100% <100%> (ø)`	`9 <9> (?)`
...n/java/io/envoyproxy/controlplane/cache/Watch.java	`100% <100%> (ø)`	`12 <7> (+2)`	⬆️
...lane/server/AdsDiscoveryRequestStreamObserver.java	`100% <100%> (ø)`	`12 <12> (?)`
...olplane/server/DiscoveryRequestStreamObserver.java	`85.36% <85.36%> (ø)`	`20 <20> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f127e26...5f91376. Read the comment docs.

sschepens · 2019-03-13T02:59:29Z

So, I ran some measurements creating 1M objects of Watch and DiscoveryRequestStreamObserver, here are the results.

Before

Watch: 20 bytes for AtomicBoolean + 49 bytes for Watch object itself. `Totalling: 69 bytes per Watch
DiscoveryRequestStreamObserver: 24 bytes for AtomicLong + 100 bytes for ConcurrentHashMap * 3 maps per observer + 89 bytes for DiscoveryRequestStreamObserver object itself. Totalling: 413 bytes per DiscoveryRequestStreamObserver

After:

Watch: 45 bytes for WatchObject itself (no other object is created except for static atomic field updater). Totalling: 45 bytes per Watch
AdsDiscoveryRequestStreamObserver: 100 bytes for ConcurrentHashMap * 3 maps per observer + 88 bytes for AdsDiscoveryRequestStreamObserver object itself. Totalling: 388 bytes per AdsDiscoveryRequestStreamObserver
XdsDiscoveryRequestStreamObserver: 88 bytes for XdsDiscoveryRequestStreamObserver object itself. Totalling: 88 bytes per XdsDiscoveryRequestStreamObserver

Conclusions:

Watch: we end up saving 24 bytes (about 34%) per each Watch created.
AdsDiscoveryRequestStreamObserver: we end up saving 25 bytes (about 6%) per each AdsDiscoveryRequestStreamObserver.
XdsDiscoveryRequestStreamObserver: we end up saving 325 bytes (about 78%) for each XdsDiscoveryRequestStreamObserver.

The savings are noticeable enough for Watches and are incredible when operating with separate XDS streams.

Of course there should also be performance improvements when operating with separate XDS streams coming from not using concurrent hash maps but plain field access.

Attaching results:

jakubdyszkiewicz

LGTM, thanks for the report on the performance improvements!

snowp · 2019-03-18T15:47:04Z

@joeyb Mind taking a look at this? I'm having a hard time finding time to review this unfortunately

sschepens · 2019-04-17T13:00:01Z

@snowp @joeyb can any of you please review this?

snowp

This makes sense, thanks.

cache/src/main/java/io/envoyproxy/controlplane/cache/Watch.java

server/src/main/java/io/envoyproxy/controlplane/server/AdsDiscoveryRequestStreamObserver.java

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java

server/src/main/java/io/envoyproxy/controlplane/server/XdsDiscoveryRequestStreamObserver.java

jakubdyszkiewicz · 2019-07-05T18:58:20Z

@sschepens can I ask whether you planning to work on this PR? I think that the improvements are valuable and it would be great to have this merged.

sschepens · 2019-07-12T14:50:42Z

@jakubdyszkiewicz yeah, wanted to follow up on this, i had the relevant changes made, but then i got my laptop replaced because of hardware issues and lost them.

I'll see if i can address the comments today

- Extract from Discovery server - Create abstract class - Create Ads observer which maintains current structure - Create simpler Xds observer with less overhead Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens · 2019-07-12T19:31:51Z

@snowp @jakubdyszkiewicz addressed comments, review?

jakubdyszkiewicz

Can you rebase to current master?

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java

…bserver

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens · 2019-07-19T14:24:47Z

@jakubdyszkiewicz updated from master

Signed-off-by: Sebastian Schepens <[email protected]>

jakubdyszkiewicz · 2019-07-25T12:15:37Z

@snowp can you look at it? Merging is blocked by "change requested"

snowp

Just a tiny nit, otherwise LGTM. Thanks for the contribution! Approving so I won't block merging

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens · 2019-07-25T18:19:21Z

@jakubdyszkiewicz @snowp addressed last comment

jakubdyszkiewicz · 2019-07-25T18:23:22Z

Thank you! Great job

sschepens · 2019-07-25T18:24:35Z

@snowp could we get a release including the latest changes?

jakubdyszkiewicz · 2019-07-25T18:27:23Z

@snowp we would like to know what's is the instruction for releasing a new version. Is there a link to it? If not, can you drop me the message at my mail (github profile) or Envoy Slack?
@sschepens it seems that there are still some conflicts.

joeyb · 2019-07-25T18:29:12Z

@jakubdyszkiewicz - I haven't written any formal docs yet, but I'll send you the release instructions on slack.

sschepens · 2019-07-25T18:32:31Z

@sschepens it seems that there are still some conflicts.

@jakubdyszkiewicz conflicts which prevent merging this? github isn't reporting any conflicts to me

jakubdyszkiewicz · 2019-07-25T18:33:43Z

It does for me

sschepens · 2019-07-25T18:34:39Z

may be try a Squash and Merge instead?

jakubdyszkiewicz reviewed Mar 12, 2019

View reviewed changes

jakubdyszkiewicz previously approved these changes Mar 13, 2019

View reviewed changes

sschepens dismissed jakubdyszkiewicz’s stale review via dbaa3f8 April 17, 2019 13:09

snowp requested changes Apr 18, 2019

View reviewed changes

sschepens added 6 commits July 12, 2019 13:00

use field updater instead of atomicboolean

2824ec5

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Replace duplicate cancel logic with method call

49af06c

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Ads streams default to ANY_TYPE_URL

f58fb74

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Style fixes

1a8602d

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

Fix ads discovery request on empty ackedresources

386a529

Signed-off-by: sschepens <[email protected]> Signed-off-by: Sebastian Schepens <[email protected]>

sschepens force-pushed the refactor-stream-observer branch from dbaa3f8 to 386a529 Compare July 12, 2019 17:33

address comments

ed46a62

Signed-off-by: Sebastian Schepens <[email protected]>

jakubdyszkiewicz reviewed Jul 19, 2019

View reviewed changes

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java Outdated Show resolved Hide resolved

sschepens added 2 commits July 19, 2019 11:21

Merge remote-tracking branch 'upstream/master' into refactor-stream-o…

d369873

…bserver

fix wording

214ddca

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens force-pushed the refactor-stream-observer branch from 495215f to 214ddca Compare July 19, 2019 14:23

remove unused updater

94dfbac

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens force-pushed the refactor-stream-observer branch from 827352f to 94dfbac Compare July 19, 2019 15:16

sschepens added 2 commits July 19, 2019 12:21

remove unused import

e2f5152

Signed-off-by: Sebastian Schepens <[email protected]>

small refactor

b81ef44

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens force-pushed the refactor-stream-observer branch from c70d576 to b81ef44 Compare July 19, 2019 16:31

make defaultTypeUrl private

3d82263

Signed-off-by: Sebastian Schepens <[email protected]>

jakubdyszkiewicz previously approved these changes Jul 23, 2019

View reviewed changes

snowp previously approved these changes Jul 25, 2019

View reviewed changes

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java Outdated Show resolved Hide resolved

sschepens dismissed stale reviews from snowp and jakubdyszkiewicz via e2c8c54 July 25, 2019 18:18

remove unnecessary line break

5f91376

Signed-off-by: Sebastian Schepens <[email protected]>

sschepens force-pushed the refactor-stream-observer branch from e2c8c54 to 5f91376 Compare July 25, 2019 18:19

jakubdyszkiewicz approved these changes Jul 25, 2019

View reviewed changes

jakubdyszkiewicz merged commit b08db6a into envoyproxy:master Jul 25, 2019

sschepens deleted the refactor-stream-observer branch July 25, 2019 18:35

server: Refactor stream observer #95

server: Refactor stream observer #95

Uh oh!

Conversation

sschepens commented Mar 12, 2019

Uh oh!

jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

jakubdyszkiewicz Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

sschepens Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sschepens commented Mar 12, 2019

Uh oh!

codecov-io commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sschepens commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After:

Conclusions:

Uh oh!

jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Mar 18, 2019

Uh oh!

sschepens commented Apr 17, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakubdyszkiewicz commented Jul 5, 2019

Uh oh!

sschepens commented Jul 12, 2019

Uh oh!

sschepens commented Jul 12, 2019

Uh oh!

jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sschepens commented Jul 19, 2019

Uh oh!

jakubdyszkiewicz commented Jul 25, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sschepens commented Jul 25, 2019

Uh oh!

jakubdyszkiewicz commented Jul 25, 2019

Uh oh!

sschepens commented Jul 25, 2019

Uh oh!

jakubdyszkiewicz commented Jul 25, 2019

Uh oh!

joeyb commented Jul 25, 2019

Uh oh!

sschepens commented Jul 25, 2019

Uh oh!

jakubdyszkiewicz commented Jul 25, 2019

Uh oh!

sschepens commented Jul 25, 2019

Uh oh!

Uh oh!

codecov-io commented Mar 13, 2019 •

edited

Loading

sschepens commented Mar 13, 2019 •

edited

Loading