search

jkyberneees / low-http-server

HTTP Server implementation in top of uWebSocket.js
MIT License
50 stars 7 forks source link

3.0 #12

Closed schamberg97 closed 3 years ago

schamberg97 commented 3 years ago

New abilities

  1. low-http-server now inherits from EventEmitter, so that frameworks relying on HTTP server being EventEmitter can now use low-http-server as backend.
  2. More Node.js HTTP API Surface is now available:
    • net.socket.remoteAddress (via HttpResponse.getRemoteAddressAsText() from uWebSockets.js)
    • http.ServerResponse.hasHeader
    • http.ServerResponse.req
    • & more
  3. Now compatible with Fastify
jkyberneees commented 3 years ago

Hi @schamberg97, the value of this contribution is huge!!!

Just a minor request from my side: It would be great to document how v3.0 compares to v.2x in relation to performance (reqs/sec), this would be a good addition to the README. I am curious if there is any performance degradation...

Many thanks.

schamberg97 commented 3 years ago

Hi @schamberg97, the value of this contribution is huge!!!

Just a minor request from my side: It would be great to document how v3.0 compares to v.2x in relation to performance (reqs/sec), this would be a good addition to the README. I am curious if there is any performance degradation...

Many thanks.

I don’t think there’s any severe performance degradation, but you are correct - this needs to be done. I will make the commit soon

schamberg97 commented 3 years ago

@jkyberneees There is in fact one change that creates a performance degradation and you were wise to ask about this. It is caused by this piece that is responsible for populating req.socket: https://github.com/jkyberneees/low-http-server/blob/1ab0a9c0ecfc257d86d2a8081982306e4e369307/src/server.js#L37-L46. This functionality isn't strictly required to use any framework, but it helps populate req.ip in fastify and similar frameworks.

According to wrk, it reduces the number of req/s from 56k to 51k per second (around 9-15% loss), when low-http-server is used without frameworks. With this piece of code removed, I couldn't reliably establish that low-http-server has become faster or slower, thus the performance has probably remained the same.

The real question is whether to go ahead with this code or remove it. I am in favour of removing it and documenting how to get remote client's IP. The third alternative is to let an end user choose, whether he needs this functionality, which will allow users to relatively painlessly migrate to low-http-server, if they need to, without sacrificing performance for the rest of users.

P.S. avoiding getter by doing: 

{

remoteAddress: ArrayBufferDecoder.decode(resWrapper.res.getRemoteAddressAsText())

}

is even worse in terms of performance.

P.P.S. If remoteAddress functionality is retained, it needs to be rewritten, as I've found a better way to do it

schamberg97 commented 3 years ago

I believe I've been able to eliminate the reason for performance regression by moving res.socket into a separate class and module.

Using ./demos/0http-basic.js under low-http-server 2.1.3 (only port number was changed):

schamberg@Nikolays-MacBook-Pro ~> wrk -t8 -c64 -d15s "http://localhost:3001/hi"
Running 15s test @ http://localhost:3001/hi
  8 threads and 64 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.07ms  194.20us   5.09ms   91.39%
    Req/Sec     7.48k   358.95     8.00k    76.41%
  899661 requests in 15.10s, 84.94MB read
Requests/sec:  59578.98
Transfer/sec:      5.63MB

Using ./demos/0http-basic.js under low-http-server 3.0.0:

schamberg@Nikolays-MacBook-Pro ~> wrk -t8 -c64 -d15s "http://localhost:3000/hi"
Running 15s test @ http://localhost:3000/hi
  8 threads and 64 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.09ms  198.44us   5.63ms   91.24%
    Req/Sec     7.35k   398.61     8.73k    73.57%
  882232 requests in 15.10s, 83.29MB read
Requests/sec:  58424.85
Transfer/sec:      5.52MB

The difference between two versions now is only 2% in favour of the previous version. However, 3.0.0 has the benefit of doubt in my mind, as this could have been caused by some thermal issues with my current MacBook Pro (might need to take it to service, I think there's dust build-up). Regardless, I'll look into more ways to increase performance.

jkyberneees commented 3 years ago

Hi @schamberg97, thanks for iterating on this PR. As I see it, any optional functionality that degrades performance should be disabled by default. However, as you are boosting compatibility with little impact on performance, I am fine for a first release.