Skip to content

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Feb 11, 2019

This improves the performance of matchKnownFields. This function is heavily used when parsing incoming http headers.

Here is the benchmark result:

                                                                             confidence improvement accuracy (*)    (**)   (***)
 http/incoming_headers.js headerDuplicates=0 c=50 benchmarker='autocannon'                   0.53 %      ±3.11% ±4.16% ±5.44%
 http/incoming_headers.js headerDuplicates=0 c=500 benchmarker='autocannon'                  2.16 %      ±3.69% ±4.95% ±6.50%
 http/incoming_headers.js headerDuplicates=20 c=50 benchmarker='autocannon'         ***     19.92 %      ±3.53% ±4.69% ±6.12%
 http/incoming_headers.js headerDuplicates=20 c=500 benchmarker='autocannon'        ***     16.96 %      ±3.89% ±5.18% ±6.74%
 http/incoming_headers.js headerDuplicates=5 c=50 benchmarker='autocannon'          ***     10.68 %      ±3.57% ±4.76% ±6.19%
 http/incoming_headers.js headerDuplicates=5 c=500 benchmarker='autocannon'           *      6.26 %      ±4.86% ±6.47% ±8.42%
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 11, 2019
@starkwang
Copy link
Contributor Author

// headers.

// 'array' header list is taken from:
// https://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpHeaderArray.cpp
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment might be outdated now? The link is dead at any rate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This link is redirected to here. But I think it might not be a good way to keep an unstable url in comment. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be okay with removing it.

if (lowercased) {
return '\u0000' + field;
} else {
return matchKnownFields(field.toLowerCase(), true);
Copy link
Member

Choose a reason for hiding this comment

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

It might be slightly faster to use iteration than recursion. it might also be a wash, however. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've run the benchmark of iteration and found there was no difference between them.

return 'expires';
case 'Set-Cookie':
case 'set-cookie':
if (field === 'If-Match' || field === 'if-Match')
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this supposed to be if-match instead of if-Match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll change it.

@starkwang
Copy link
Contributor Author

@starkwang
Copy link
Contributor Author

Landed in da0dc51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants