Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring Rx-placement and Rx-mode #1344

Merged
merged 16 commits into from
May 17, 2019
Merged

Refactoring Rx-placement and Rx-mode #1344

merged 16 commits into from
May 17, 2019

Conversation

milanlenco
Copy link
Collaborator

@milanlenco milanlenco commented May 16, 2019

The original implementation:

  • didn't allow to set rx-placement and rx-mode for multiple queues at once
  • didn't work correctly with memifs - placement and mode cannot be set until memif has established connection, because only then the queues are created are ready to accept the configuration (therefore interface link state has to be watched and scheduler needs to be notified about changes)

As always I have also included small cleanup and bug fixing from inside kvscheduler...

@milanlenco milanlenco added the 🚧 WIP do not merge! work in progress! label May 16, 2019
@milanlenco milanlenco self-assigned this May 16, 2019
@@ -111,8 +112,11 @@ func (reg *registry) GetDescriptorForKey(key string) *KVDescriptor {
var keyDescriptor *KVDescriptor
for _, descriptor := range reg.descriptors {
if descriptor.KeySelector(key) {
if keyDescriptor != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Was just thinking if this is actually not gonna make GetDescriptorForKey more expensive performance-wise. Can you run the perf tests with 20k to compare difference?

Copy link
Collaborator Author

@milanlenco milanlenco May 17, 2019

Choose a reason for hiding this comment

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

It varies quite a bit on my system, but there doesn't seem to be measurable drop in the performance:

BEFORE:

level=info msg="Client #0 done, 20000 tunnels took 1m26.308s" loc="grpc-perf/main.go(302)" logger=grpc-stress-test-client
level=info msg="All clients done, took: 1m16.7757s" loc="grpc-perf/main.go(219)" logger=grpc-stress-test-client
level=info msg="All clients done, took: 1m15.096s" loc="grpc-perf/main.go(219)" logger=grpc-stress-test-client

AFTER:

level=info msg="All clients done, took: 1m5.0734s" loc="grpc-perf/main.go(219)" logger=grpc-stress-test-client
level=info msg="All clients done, took: 1m18.1164s" loc="grpc-perf/main.go(219)" logger=grpc-stress-test-client
level=info msg="All clients done, took: 1m12.1412s" loc="grpc-perf/main.go(219)" logger=grpc-stress-test-client

This lookup is cached (for the last 500 different keys), so effectively each distinct key goes through the cycle only once and the number of descriptors isn't that significant.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, merging then.

Copy link
Member

@ondrej-fabry ondrej-fabry left a comment

Choose a reason for hiding this comment

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

LGTM.

@ondrej-fabry ondrej-fabry merged commit 6240579 into ligato:dev May 17, 2019
@ondrej-fabry ondrej-fabry added this to the v2.2.0 milestone May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants