Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Use steed.each instead of plain loop #153

Closed
wants to merge 4 commits into from
Closed

Conversation

behrad
Copy link
Contributor

@behrad behrad commented Jun 16, 2016

No description provided.

behrad added 2 commits June 16, 2016 16:38
merge with upstream master
use steed.each instead of for loop
@mcollina
Copy link
Collaborator

tests are failing on all envs :/

@behrad
Copy link
Contributor Author

behrad commented Jun 16, 2016

Was editing this from github web editor, my mistake to remove defer(done) :)
Should now be fixed :)

@coveralls
Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage increased (+0.01%) to 92.434% when pulling 543f64a on adpdigital:master into 997de4a on mcollina:master.

setImmediate(function(){
cb(topic, message, options);
});
next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you using setImmediate there? And why are you calling done() before the actual callback is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering a huge (multi hundred thousands) list of forwarder callbacks, Does't setImmediate improve the iteration?Or steed does the same job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can bring next inside setImmediate right after cb returned. I just felt slow client callbacks should be don't care! so that we can report back from .publish ASAP!

Copy link
Collaborator

Choose a reason for hiding this comment

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

introducing setImmediate there will increase the total latency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change this, thank you

@mcollina
Copy link
Collaborator

I still do not understand why using steed.each will be faster than just a plain loop.

@behrad
Copy link
Contributor Author

behrad commented Jun 16, 2016

I believe it is related to #148

@mcollina
Copy link
Collaborator

Solving this issue will be more complicated that a simple fix. You probably would want to have a queue-like thing. Using setImmediate will just create a very high load on the node.js process. A much easier solution is to reduce the cost of sending a message, i.e. move to mqemitter and aedes.

@behrad
Copy link
Contributor Author

behrad commented Jun 16, 2016

For the move, I'm waiting for aedes to be production ready, and I can help you anything around this.
For the queue, may we need that also in aedes?

@mcollina
Copy link
Collaborator

@behrad how many packets are you delivering? Aedes can deliver roughly 100k msg/s (mosca/ascoltatori 25k msg/s).

@behrad
Copy link
Contributor Author

behrad commented Jun 16, 2016

multiple hundred thousands is my target.
However with my own app logic on top of embedded mosca (haven't had enough time to separate that into multiple processes) it seems around 10K-15K in my local network, when 100K clients connected.
I'm also running many mosca instances (both horizontal & vertical=using pm2)!

@behrad behrad closed this Aug 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants