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

New connection parser #113

Closed
wants to merge 4 commits into from
Closed

New connection parser #113

wants to merge 4 commits into from

Conversation

mcollina
Copy link
Member

I was able to get some better performance numbers from our poor Connection class.

@adamvr could you please review it?

I'm getting something near 10% increase.

mcollina added 3 commits June 14, 2013 10:48
The new implementation is more performant and "sane".
Moreover, it required a refactoring of the reconnection logic, which has
lead to a more 'stable' algorithm.
@mcollina
Copy link
Member Author

@adamvr, If it is not a problem I'll release it as 0.3.0 over the weekend

@adamvr
Copy link
Member

adamvr commented Jun 24, 2013

@mcollina I'm unsure about the stream builder thing. Is there actually a problem with reusing the same socket?

@mcollina
Copy link
Member Author

You were doing it wrong before. Node.js sockets retain same state when they close, and they cannot be piped again. The connect method is not really supposed to be used that way :(.

@adamvr
Copy link
Member

adamvr commented Jun 24, 2013

Aha, okay. I guess I'll defer to you then. Make it so, Mr. Collina!

@adamvr
Copy link
Member

adamvr commented Jun 24, 2013

p.s. node_redis just calls connect again on the socket as far as I can tell. See https://github.com/mranney/node_redis/blob/master/index.js#L479-494

@mcollina
Copy link
Member Author

Maybe it is a node v0.10/stream2 thing, I am not sure.
Still, you cannot pipe a stream after it has been closed :(.

@mcollina
Copy link
Member Author

Do you think it is better to release it as 0.3.0, or just as the next in the 0.2.x series?

@adamvr
Copy link
Member

adamvr commented Jun 26, 2013

Is it actually necessary to have a full pipe, though? I mean, the only information the parser needs to know from the stream is the data coming out of it, not whether or not the stream is connected, disconnected, closing, piping or whatever. I'm having good (passing tests) results just doing something like:

this.stream.on('readable', function() {
   var chunk = that.stream.read();
   that.parser.write(chunk);
}

Any chance you could gist your benchmarks so I can test that against them?

As to the version number, I don't know. I would probably put it in the 0.2.x since it isn't an API changing change (externally, anyway. I'm sure the few if any people who do new MqttClient are going to be right confused), but you're right that we probably should have bumped reconnection/offline into 0.3.0.

@mcollina
Copy link
Member Author

The real difference is that by using pipe you handle the "pressure" on the stream, e.g. if it has not completed then it's not called again, and there is some good old buffering (which is good, btw).
My benches are there: https://github.com/mcollina/mosca/blob/master/benchmarks/bombing.js and https://github.com/mcollina/mosca/blob/master/benchmarks/throughputCounter.js

@mcollina mcollina mentioned this pull request Jul 20, 2013
5 tasks
@mcollina
Copy link
Member Author

Already included in #118.

@mcollina mcollina closed this Jul 20, 2013
@YoDaMa YoDaMa deleted the new-connection branch May 11, 2020 06:16
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.

2 participants