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

No limit on the message queue size, #23

Merged
merged 4 commits into from
Jun 3, 2015

Conversation

klauspost
Copy link
Contributor

but instead disconnect users if it takes too long to send a message.

Continuation of #19.

TODO:

…t takes too long to send a message.

Continuation of centrifugal#19.
@FZambia
Copy link
Member

FZambia commented Jun 2, 2015

Just ran a benchmark.

First – without ring queue

First column - amount of connected clients, second - nanoseconds between moment when one message sent into channel and moment when all clients received that message from channel:

MacAir:benchmarks fz$ go run benchmark.go ws://127.0.0.1:8000/connection/websocket development secret 10000 1000 30
1433269899
max clients: 10000
increment: 1000
repeat: 30
1000    16708499
2000    30078912
3000    41459100
4000    54806012
5000    74290028
6000    91229309
7000    104538608
8000    112292043
9000    121811160
10000   148505642

Memory usage: 581mb

And with queue:

MacAir:benchmarks fz$ go run benchmark.go ws://127.0.0.1:8000/connection/websocket development secret 10000 1000 30
1433270163
max clients: 10000
increment: 1000
repeat: 30
1000    22178297
2000    43288456
3000    53741986
4000    78144522
5000    94671665
6000    114886631
7000    138078154
8000    155005984
9000    178497946
10000   185657515

Memory: 564mb

As we can see memory usage reduced a little and performance for this task ~25% worse. GOMAXPROCS=1.

With GOMAXPROCS=2 which is optimal for my old Mac Air:

Without queue:

MacAir:benchmarks fz$ go run benchmark.go ws://127.0.0.1:8000/connection/websocket development secret 10000 1000 30
1433271313
max clients: 10000
increment: 1000
repeat: 30
1000    13462864
2000    22556269
3000    31261169
4000    46361146
5000    61516916
6000    69018288
7000    70037870
8000    80599602
9000    81334544
10000   107770691

With queue:

MacAir:benchmarks fz$ go run benchmark.go ws://127.0.0.1:8000/connection/websocket development secret 10000 1000 30
1433271109
max clients: 10000
increment: 1000
repeat: 30
1000    13935637
2000    24814715
3000    38827647
4000    50307733
5000    59065724
6000    67227032
7000    87391316
8000    91397998
9000    113766341
10000   122585935

Memory usage again a bit less, performance impact ~15%

What do you think about this results? It's a bit upset to lose 20% but we theoretically get more predictable system

klauspost added a commit to klauspost/centrifugo that referenced this pull request Jun 2, 2015
Benchmarks the raw SubHub performance as well as client routing performance.

Right now it will usually crash with internal server error until centrifugal#19 and centrifugal#23 is fixed.
@klauspost
Copy link
Contributor Author

Very interesting. I think the main difference comes from the send timeout. What are the numbers if you change the function to:

func (c *client) sendMsgTimeout(msg string) error {
    return c.sess.Send(msg)
}

I think this would make it just as fast as the "old" code, but clients will no longer time out on send.

@FZambia
Copy link
Member

FZambia commented Jun 2, 2015

Yep (compare with top most result GOMAXPROCS=1, no queue)

max clients: 10000
increment: 1000
repeat: 30
1000    17974868
2000    30902744
3000    42228004
4000    58556314
5000    74047742
6000    81232314
7000    100688710
8000    107233599
9000    128464179
10000   133358961

@klauspost
Copy link
Contributor Author

The buffer and separate goroutine can now also be removed from websocket. See commit 3bc068d - should speed up websocket quite a bit.

So the question is, do we want:

  • Timeouts on client connections?
  • A fixed limit on queue size?

FZambia added a commit that referenced this pull request Jun 3, 2015
@FZambia FZambia merged commit 118f63b into centrifugal:master Jun 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants