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

fetch: replace HeadersList Array inheritance to Map #1309

Merged
merged 12 commits into from
Apr 9, 2022
Merged

fetch: replace HeadersList Array inheritance to Map #1309

merged 12 commits into from
Apr 9, 2022

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Mar 28, 2022

This PR replaces the HeadersList Array inheritance to Map, which basically is more suitable for headers use case. It also increases the performance by 10% (it's not done yet, so it might be more/less).

lib/fetch/headers.js Outdated Show resolved Hide resolved
lib/fetch/headers.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member

ronag commented Mar 29, 2022

@Ethan-Arrowood

@Ethan-Arrowood
Copy link
Collaborator

From my investigations the Map wasn't as good for reading the header values (as in per spec you must return the values in sorted order) but I do know v8 is always making magic happen; especially with objects like Map.

Wouldn't be shocked if your new benchmark results are even better perf wise.

One thing to keep in mind is memory footprint too; a flat array of strings should be more efficient space wise but it's a hard one to verify as well.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@RafaelGSS
Copy link
Member Author

as in per spec you must return the values in sorted order

Can you point me to this statement in the spec? Yeah, in my benchmarks it was better than Array implementation, mostly due to the fact of needing binarySearch to do any operation in headers using Array inheritance

@ronag
Copy link
Member

ronag commented Mar 29, 2022

as in per spec you must return the values in sorted order

Can you point me to this statement in the spec? Yeah, in my benchmarks it was better than Array implementation, mostly due to the fact of needing binarySearch to do any operation in headers using Array inheritance

https://fetch.spec.whatwg.org/#headers-class

The value pairs to iterate over are the return value of running sort and combine with this’s header list.

@AlttiRi
Copy link

AlttiRi commented Mar 29, 2022

Just run a server that logs the incoming request's headers:

For example, fetch of Chrome:
Chrome headers

The headers are not alphabetically ordered.
Neither in requests from fetch function, nor in any other request from the browser.

Also I don't see any clear mention in the specs that the headers should be alphabetically ordered.

BTW, an example of "sorted" array: ["1", "2", "3", "4", "5"].sort(() => Math.random() >= 0.5 ? -1 : 1).


I expect that it (undici/fetch) will allow to a user decide the order of the headers, if the user will decide to overwrite the default headers, or add new ones.

@ronag
Copy link
Member

ronag commented Mar 29, 2022

Yea. I think the spec only requires it to be sorted in the Headers class iterator member function.

@RafaelGSS
Copy link
Member Author

In fact, the iteration is sorted:

image

I'll adjust the PR to match this requirement.

@RafaelGSS
Copy link
Member Author

I have adjusted, but still non-optimal. I'll implement a orderedmap soon.

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2022

Codecov Report

Attention: Patch coverage is 70.83333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 94.10%. Comparing base (65feb05) to head (f604e28).
Report is 1425 commits behind head on main.

Files with missing lines Patch % Lines
lib/fetch/headers.js 76.92% 9 Missing ⚠️
lib/fetch/response.js 28.57% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1309      +/-   ##
==========================================
- Coverage   94.16%   94.10%   -0.06%     
==========================================
  Files          45       45              
  Lines        4112     4189      +77     
==========================================
+ Hits         3872     3942      +70     
- Misses        240      247       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/fetch/headers.js Outdated Show resolved Hide resolved
lib/fetch/headers.js Outdated Show resolved Hide resolved
lib/fetch/headers.js Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Would it make sense to cache the sorted list?

@RafaelGSS
Copy link
Member Author

Would it make sense to cache the sorted list?

Yes, I thought about it yesterday. However, I'm thinking of a solution for an insertion sort (like the one before, but with less overhead).

Also, it's better(for the memory footprint) to have a flag telling if the list is currently sorted. Example:

if (this[kSorted]) {
  listFreezed = new HeadersList(this[kHeadersList])
} else {
  this[kHeadersList] = new HeadersList([...this[kHeadersList]].sort())
  this[kSorted] = true
  listFreezed = new HeadersList(this[kHeadersList])
}

for (const val of listFreezed.values()) {
  yield val
}

@ronag
Copy link
Member

ronag commented Apr 6, 2022

@RafaelGSS Please don't stop working on this. 😄 I was looking forward to having this fixed.

@RafaelGSS
Copy link
Member Author

@RafaelGSS Please don't stop working on this. smile I was looking forward to having this fixed.

Sure thing! I just have some priorities for this week. I'll be back on it soon as possible 😄

@RafaelGSS RafaelGSS marked this pull request as ready for review April 8, 2022 02:56
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Getting close!

This should not require any (almost) changes outside of the headers stuff, e.g. replacing append with set is wrong and unnecessary.

Also the store for the headers should never be sorted. You would need to create a separate sorted list copy used only for iteration. We want to send the headers to the server as specified by the users (i.e. without sorting).

lib/fetch/headers.js Outdated Show resolved Hide resolved
@RafaelGSS
Copy link
Member Author

Also the store for the headers should never be sorted. You would need to create a separate sorted list copy used only for iteration. We want to send the headers to the server as specified by the users (i.e. without sorting).

Good spot. Working on this.

lib/fetch/headers.js Outdated Show resolved Hide resolved
@RafaelGSS
Copy link
Member Author

RafaelGSS commented Apr 8, 2022

@ronag After all the changes, looks like we will get no performance improvement. The Benchmark CI looks not consistent, so, I'll perform those tests in my dedicated machine and share the results soon.

In case of no performance improvements, I'm not sure how to proceed with that. Maybe just close it?

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Apr 8, 2022

Main:

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
│ undici - fetch      │      35 │ 1391.03 req/sec │  ± 2.80 % │                       - │
│ undici - fetch      │      25 │ 1455.34 req/sec │  ± 2.82 % │                       - │
│ undici - fetch      │      30 │ 1400.48 req/sec │  ± 2.62 % │                       - │

This branch:

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
│ undici - fetch      │      15 │ 1414.86 req/sec │  ± 2.38 % │                       - │
│ undici - fetch      │      10 │ 1394.15 req/sec │  ± 2.37 % │                       - │
│ undici - fetch      │      10 │ 1431.36 req/sec │  ± 2.62 % │                       - │

TL;DR: The performance is the same, we should just choose the best implementation.

@ronag
Copy link
Member

ronag commented Apr 8, 2022

I still think this PR is good since it's more correct in terms of the ordering @AlttiRi mentions.

const normalizedValue = normalizeAndValidateHeaderValue(name, value)
* [Symbol.iterator] () {
if (!this[kHeadersSortedMap]) {
this.sortMap()
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 you can inline sortMap?

Copy link
Member Author

@RafaelGSS RafaelGSS Apr 8, 2022

Choose a reason for hiding this comment

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

You mean?

if (!this[kHeadersSortedMap]) this.sortMap()

lib/fetch/headers.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, the perf boost would bave been nice

@ronag
Copy link
Member

ronag commented Apr 9, 2022

@RafaelGSS I helped a little. Hope you don't mind. PTAL.

@RafaelGSS
Copy link
Member Author

LGTM overall.

@ronag ronag merged commit c0961f4 into nodejs:main Apr 9, 2022
@RafaelGSS
Copy link
Member Author

This is a breaking change, right?

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* update: uses Map over Array in HeadersList

* fix: create empty HeaderList

* update(fetch/headers): sort keys before returning

* update: remove binarySearch method

* perf: use headersList.set to bypass validation

* update: remove comment

* fix: lint

* update(headers): create a new headers map for sorting

* update: remove kSorted

* update: replace .set to .append

* fixes

* fix: lint

Co-authored-by: Robert Nagy <ronagy@icloud.com>
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* update: uses Map over Array in HeadersList

* fix: create empty HeaderList

* update(fetch/headers): sort keys before returning

* update: remove binarySearch method

* perf: use headersList.set to bypass validation

* update: remove comment

* fix: lint

* update(headers): create a new headers map for sorting

* update: remove kSorted

* update: replace .set to .append

* fixes

* fix: lint

Co-authored-by: Robert Nagy <ronagy@icloud.com>
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* update: uses Map over Array in HeadersList

* fix: create empty HeaderList

* update(fetch/headers): sort keys before returning

* update: remove binarySearch method

* perf: use headersList.set to bypass validation

* update: remove comment

* fix: lint

* update(headers): create a new headers map for sorting

* update: remove kSorted

* update: replace .set to .append

* fixes

* fix: lint

Co-authored-by: Robert Nagy <ronagy@icloud.com>
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.

6 participants