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

Extend send method #90

Merged
merged 13 commits into from
May 27, 2020
Merged

Extend send method #90

merged 13 commits into from
May 27, 2020

Conversation

jkyberneees
Copy link
Collaborator

@jkyberneees jkyberneees commented May 26, 2020

Extend send method to support the following data types:

  • String
  • Object
  • Error
  • Buffer (with default header)
  • Stream

Out of scope:

  • Promise

@jkyberneees jkyberneees mentioned this pull request May 26, 2020
6 tasks
@jkyberneees jkyberneees changed the title WIP: Extend send method Extend send method May 26, 2020
@@ -62,9 +64,11 @@ module.exports.send = (options, req, res) => {
}

if (data) {
if (data instanceof Buffer) {
if (typeof data === 'string') {
contentType = contentType || TYPE_PLAIN
Copy link
Contributor

Choose a reason for hiding this comment

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

with this we are losing performance, I put in separate functions because directly towards its objective, reassigning parameters is not a good practice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Jesus, we are not loosing performance with this, we are gaining 2-4k more requests per second with this updates. I agree, this is not probable nice looking or good practices, but this is very performant, at least according to my stress tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkyberneees We should write those tests too, so we can have better quality code.

sendError(res, data, cb)
return
}
const [statusCode, errStr] = parseErr(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

sendError does the whole process, it does not return an array so that a destructuring is performed later, this is not good if we want to improve performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please see my updates, intermediate functions does not end the request, just update the data object. (with exception of the stream support).
I will update this code to actually remove the usage of destructing.

@jkyberneees
Copy link
Collaborator Author

Just added a performance benchmark to monitor res.send regressions:

% node performance/suites/res-send.js     
[
  { name: 'statusCode', mean: '0.0000000013' },
  { name: 'undefined', mean: '0.0000000025' },
  { name: 'null', mean: '0.0000000053' },
  { name: 'string', mean: '0.0000000055' },
  { name: 'stream', mean: '0.0000000270' },
  { name: 'buffer', mean: '0.0000000278' },
  { name: 'string + headers', mean: '0.0000000585' },
  { name: 'json', mean: '0.0000002893' },
  { name: 'error', mean: '0.0000003384' },
  { name: 'json + headers', mean: '0.0000003664' }
]

Machine: Macbook Pro 13 2020 - 1,4 GHz Quad-Core Intel Core i5

cc @jesusvilla

@jkyberneees jkyberneees merged commit f3d1b71 into master May 27, 2020
@jkyberneees jkyberneees deleted the extend-send-method branch May 27, 2020 16:53
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