celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

Buffer incoming data from the socket #56

Closed rakoo closed 11 years ago

rakoo commented 11 years ago

Buffer pipelined requests from the socket coming in so that they can be parsed the next time we are ready to use a Reel::Request.

rakoo commented 11 years ago

This one fixes #55.

coveralls commented 11 years ago

Coverage Status

Coverage increased (+14.04%) when pulling 6b7f41f21602b6c51d61796b13d21829d6bd2875 on rakoo:fix-pipelining into 5a7f2a560f35a2ee2d16717f57d3d14c7c95a14a on celluloid:master.

coveralls commented 11 years ago

Coverage Status

Coverage increased (+14.04%) when pulling 6b7f41f21602b6c51d61796b13d21829d6bd2875 on rakoo:fix-pipelining into 5a7f2a560f35a2ee2d16717f57d3d14c7c95a14a on celluloid:master.

tarcieri commented 11 years ago

This looks interesting, but I'd like to see how it affects performance in a typical non-pipelined keepalive case. This is adding an awful lot of logic/buffering to the normal request path.

rakoo commented 11 years ago

Agreed, both the buffer juggling and the char-by-char look awful. Something interesting would be for the http parser to register the number of bytes parsed since initialization, so that in the case of normal requests we know there is nothing to buffer, only verify it's empty before parsing what's coming from the socket.

I see you're also the maintainer of the http gem, is that something possible ?

digitalextremist commented 11 years ago

@rakoo isn't it the parser ( tmm1/http_parser.rb ) you want to affect though, maintained by @tmm1?

rakoo commented 11 years ago

@digitalextremist you are right, the parser is another gem, and a whole another beast.

digitalextremist commented 11 years ago

@rakoo beast is the best possible word you could have chosen as the parser's descriptor :)

tarcieri commented 11 years ago

Sorry I haven't had time to look at this... I need to see how much of a perf regression it brings about and if there are ways we can better optimize it before merging

tarcieri commented 11 years ago

This was fixed by #73. Closing this PR as it's obsolete