fetch: replace HeadersList Array inheritance to Map #1309

RafaelGSS · 2022-03-28T19:59:40Z

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

ronag · 2022-03-29T04:25:28Z

@Ethan-Arrowood

Ethan-Arrowood · 2022-03-29T04:36:38Z

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.

mcollina

lgtm

RafaelGSS · 2022-03-29T13:02:10Z

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 · 2022-03-29T14:20:39Z

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 · 2022-03-29T15:43:06Z

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

For example, fetch of Chrome:

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 · 2022-03-29T16:03:23Z

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

RafaelGSS · 2022-03-30T20:35:25Z

In fact, the iteration is sorted:

I'll adjust the PR to match this requirement.

RafaelGSS · 2022-03-31T14:10:44Z

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

codecov-commenter · 2022-03-31T14:13:13Z

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

ronag

Would it make sense to cache the sorted list?

RafaelGSS · 2022-03-31T14:43:33Z

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 · 2022-04-06T13:22:55Z

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

RafaelGSS · 2022-04-06T13:26:33Z

@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 😄

ronag

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

RafaelGSS · 2022-04-08T12:10:48Z

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

RafaelGSS · 2022-04-08T14:41:45Z

@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 · 2022-04-08T14:54:11Z

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 · 2022-04-08T15:16:02Z

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

ronag · 2022-04-08T15:16:33Z

lib/fetch/headers.js

-    const normalizedValue = normalizeAndValidateHeaderValue(name, value)
+  * [Symbol.iterator] () {
+    if (!this[kHeadersSortedMap]) {
+      this.sortMap()


I think you can inline sortMap?

You mean?

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

lib/fetch/headers.js

mcollina

lgtm, the perf boost would bave been nice

ronag · 2022-04-09T09:58:01Z

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

lib/fetch/headers.js

RafaelGSS · 2022-04-09T13:05:42Z

LGTM overall.

RafaelGSS · 2022-04-09T20:34:50Z

This is a breaking change, right?

* 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>

RafaelGSS added 2 commits March 28, 2022 16:15

update: uses Map over Array in HeadersList

8830b33

fix: create empty HeaderList

7622f76

RafaelGSS commented Mar 28, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

lib/fetch/headers.js Outdated Show resolved Hide resolved

mcollina approved these changes Mar 29, 2022

View reviewed changes

RafaelGSS mentioned this pull request Mar 29, 2022

Can't remove default headers. Can't overwrite "sec-fetch-mode", "connection" headers. #1305

Closed

update(fetch/headers): sort keys before returning

adca288

ronag reviewed Mar 31, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

ronag reviewed Mar 31, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

ronag reviewed Mar 31, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

ronag reviewed Mar 31, 2022

View reviewed changes

RafaelGSS added 3 commits April 7, 2022 23:47

update: remove binarySearch method

a43ad67

perf: use headersList.set to bypass validation

61b613d

update: remove comment

2986c98

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

fix: lint

0941bb5

ronag requested changes Apr 8, 2022

View reviewed changes

ronag reviewed Apr 8, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

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

aceec4d

ronag reviewed Apr 8, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

update: remove kSorted

765fc31

ronag reviewed Apr 8, 2022

View reviewed changes

lib/fetch/headers.js Outdated Show resolved Hide resolved

update: replace .set to .append

0fdf0a6

mcollina approved these changes Apr 9, 2022

View reviewed changes

fixes

6065226

RafaelGSS commented Apr 9, 2022

View reviewed changes

lib/fetch/headers.js Show resolved Hide resolved

fix: lint

f604e28

ronag merged commit c0961f4 into nodejs:main Apr 9, 2022

KhafraDev mentioned this pull request Apr 11, 2022

feat(rest): use undici discordjs/discord.js#7747

Merged

4 tasks

RafaelGSS mentioned this pull request Apr 17, 2022

fetch() performance parity #1203

Closed

panva mentioned this pull request May 1, 2022

[fetch] 5.1.0 regressed ability to intercept headers #1392

Closed

matthewlilley mentioned this pull request May 2, 2022

Node v14 support graphprotocol/graph-client#34

Closed

AlttiRi mentioned this pull request May 31, 2022

Keep headers order while sending them node-fetch/node-fetch#1569

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fetch: replace HeadersList Array inheritance to Map #1309

fetch: replace HeadersList Array inheritance to Map #1309

RafaelGSS commented Mar 28, 2022 •

edited

Loading

ronag commented Mar 29, 2022

Ethan-Arrowood commented Mar 29, 2022

mcollina left a comment

RafaelGSS commented Mar 29, 2022

ronag commented Mar 29, 2022

AlttiRi commented Mar 29, 2022 •

edited

Loading

ronag commented Mar 29, 2022

RafaelGSS commented Mar 30, 2022

RafaelGSS commented Mar 31, 2022

codecov-commenter commented Mar 31, 2022 •

edited

Loading

ronag left a comment

RafaelGSS commented Mar 31, 2022

ronag commented Apr 6, 2022

RafaelGSS commented Apr 6, 2022

ronag left a comment •

edited

Loading

RafaelGSS commented Apr 8, 2022

RafaelGSS commented Apr 8, 2022 •

edited

Loading

RafaelGSS commented Apr 8, 2022 •

edited

Loading

ronag commented Apr 8, 2022

ronag Apr 8, 2022

RafaelGSS Apr 8, 2022 •

edited

Loading

mcollina left a comment

ronag commented Apr 9, 2022

RafaelGSS commented Apr 9, 2022

RafaelGSS commented Apr 9, 2022

fetch: replace HeadersList Array inheritance to Map #1309

fetch: replace HeadersList Array inheritance to Map #1309

Conversation

RafaelGSS commented Mar 28, 2022 • edited Loading

ronag commented Mar 29, 2022

Ethan-Arrowood commented Mar 29, 2022

mcollina left a comment

Choose a reason for hiding this comment

RafaelGSS commented Mar 29, 2022

ronag commented Mar 29, 2022

AlttiRi commented Mar 29, 2022 • edited Loading

ronag commented Mar 29, 2022

RafaelGSS commented Mar 30, 2022

RafaelGSS commented Mar 31, 2022

codecov-commenter commented Mar 31, 2022 • edited Loading

Codecov Report

ronag left a comment

Choose a reason for hiding this comment

RafaelGSS commented Mar 31, 2022

ronag commented Apr 6, 2022

RafaelGSS commented Apr 6, 2022

ronag left a comment • edited Loading

Choose a reason for hiding this comment

RafaelGSS commented Apr 8, 2022

RafaelGSS commented Apr 8, 2022 • edited Loading

RafaelGSS commented Apr 8, 2022 • edited Loading

ronag commented Apr 8, 2022

ronag Apr 8, 2022

Choose a reason for hiding this comment

RafaelGSS Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Apr 9, 2022

RafaelGSS commented Apr 9, 2022

RafaelGSS commented Apr 9, 2022

RafaelGSS commented Mar 28, 2022 •

edited

Loading

AlttiRi commented Mar 29, 2022 •

edited

Loading

codecov-commenter commented Mar 31, 2022 •

edited

Loading

ronag left a comment •

edited

Loading

RafaelGSS commented Apr 8, 2022 •

edited

Loading

RafaelGSS commented Apr 8, 2022 •

edited

Loading

RafaelGSS Apr 8, 2022 •

edited

Loading