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

[runtime] - push performance issue #730

Closed
PavelR577 opened this issue Nov 13, 2012 · 11 comments
Closed

[runtime] - push performance issue #730

PavelR577 opened this issue Nov 13, 2012 · 11 comments
Labels

Comments

@PavelR577
Copy link

This is my scenario:
In our app we have a channel for each user so let's say that if user1 browse through the website in serveral tabs/browsers he we'll have several resources under channel names "user1".

each user get's streaming data of forex quotes, this can be intensive (about 10 broadcasts per second), also these messages are not cached (we don't need it to be cached).

Going over the logic of the broadcaster I see in method "push" this line of code:

 // We need to synchronize t make sure there is no suspend operation retrieving cached messages concurrently.
        // https://github.com/Atmosphere/atmosphere/issues/170
        synchronized (concurrentSuspendBroadcast) {

That means that each time the user refreshed/goes to another page the channel is locked , to readd the resource again, when it's locked the messages can't be sent, so in our scenario the Broadcaster future just climbs and can't keep up with the load. probably no one has experienced this issue since we have a non usual case.

So my thought is, since we don't cache the messages anyway, we can actually skip this synchronization.

Our load is about 1000 channels, and each is receiving forex quote updates (quick math - 1000*10=up to 10,000 broadcasts per second)

Is it possible to add a switch for this logic?

@jfarcand
Copy link
Member

OK here is my proposal, I will put the code inside a new method so you can override that method and don't synchronize. That means you will have your own broadcaster and you will define it in web.xml.

@PavelR577
Copy link
Author

That will be great (I actually tried to override this method but the "recentActivity" in the code is private so I can't do it)

Thanks.

@PavelR577
Copy link
Author

Basically I just need it to be written under 2 methods:

push()
push_synchronized()

so I can override the "push_synchronized" and call the "push" instead.

@PavelR577
Copy link
Author

any updates?

@jfarcand
Copy link
Member

Swapped by my daily work. Should have something before the end of the day!

jfarcand added a commit that referenced this issue Nov 15, 2012
@PavelR577
Copy link
Author

Great! I'll start testing.

Thanks again.

jfarcand added a commit that referenced this issue Nov 15, 2012
@PavelR577
Copy link
Author

fixed. now I found another issue - the JRockit show me that perRequestFilter has high contention, I see another synchronized blocked there - its synchronizing the resource which getAsyncWriteHandler also synchronizes.

What's the reason for the synchronization in the perRequestFilter?

@jfarcand
Copy link
Member

Salut, the reason is an AtmosphereResource can always be modified by a filter. I've pushed a small optmization, you can pull and try it.

978ec4c

@PavelR577
Copy link
Author

got it.

I've actually already tried removing it by myself and tested it on our prod server and it solved the load issue almost completely, so this is actually where the issue was (we don't have any filters).

I'll update to your version :) .

Thanks.

@jfarcand
Copy link
Member

Great! When do you need a release? more important, when will I get my beers for helping :-)

@PavelR577
Copy link
Author

I'll use my version until you do a release, still have a few more issues to solve... I'll update you as soon as I'll figure out how to reproduce...

As to the beers - yeah I know I owe you a lot of them already :) need to figure out a way how to send it to you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants