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

perf: optimize Iterator #2692

Merged
merged 5 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions lib/fetch/formdata.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

const { isBlobLike, toUSVString, makeIterator } = require('./util')
const { kState } = require('./symbols')
const { kEnumerableProperty } = require('../core/util')
const { File: UndiciFile, FileLike, isFileLike } = require('./file')
const { webidl } = require('./webidl')
const { Blob, File: NativeFile } = require('node:buffer')
const { File: NativeFile } = require('node:buffer')

/** @type {globalThis['File']} */
const File = NativeFile ?? UndiciFile
Expand Down Expand Up @@ -158,29 +159,32 @@ class FormData {
webidl.brandCheck(this, FormData)

return makeIterator(
() => this[kState].map(pair => [pair.name, pair.value]),
() => this[kState],
'FormData',
'key+value'
'key+value',
'name', 'value'
)
}

keys () {
webidl.brandCheck(this, FormData)

return makeIterator(
() => this[kState].map(pair => [pair.name, pair.value]),
() => this[kState],
'FormData',
'key'
'key',
'name', 'value'
)
}

values () {
webidl.brandCheck(this, FormData)

return makeIterator(
() => this[kState].map(pair => [pair.name, pair.value]),
() => this[kState],
Comment on lines -181 to +184
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the reason why the performance of the loop is poor.

Copy link
Member

Choose a reason for hiding this comment

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

I actually think we need to create a copy...

'FormData',
'value'
'value',
'name', 'value'
)
}

Expand All @@ -200,14 +204,25 @@ class FormData {
}

for (const [key, value] of this) {
callbackFn.apply(thisArg, [value, key, this])
callbackFn.call(thisArg, value, key, this)
Copy link
Member Author

Choose a reason for hiding this comment

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

Function#call faster than Function#apply.

}
}
}

FormData.prototype[Symbol.iterator] = FormData.prototype.entries

Object.defineProperties(FormData.prototype, {
append: kEnumerableProperty,
delete: kEnumerableProperty,
get: kEnumerableProperty,
getAll: kEnumerableProperty,
has: kEnumerableProperty,
set: kEnumerableProperty,
entries: kEnumerableProperty,
keys: kEnumerableProperty,
values: kEnumerableProperty,
forEach: kEnumerableProperty,
[Symbol.iterator]: { enumerable: false },
KhafraDev marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Symbol.iterator]: { enumerable: false },

[Symbol.toStringTag]: {
value: 'FormData',
configurable: true
Expand All @@ -225,7 +240,7 @@ function makeEntry (name, value, filename) {
// 1. Set name to the result of converting name into a scalar value string.
// "To convert a string into a scalar value string, replace any surrogates
// with U+FFFD."
// see: https://nodejs.org/dist/latest-v18.x/docs/api/buffer.html#buftostringencoding-start-end
// see: https://nodejs.org/dist/latest-v20.x/docs/api/buffer.html#buftostringencoding-start-end
name = Buffer.from(name).toString('utf8')

// 2. If value is a string, then set value to the result of converting
Expand Down
35 changes: 10 additions & 25 deletions lib/fetch/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,48 +492,33 @@ class Headers {
keys () {
webidl.brandCheck(this, Headers)

if (this[kGuard] === 'immutable') {
const value = this[kHeadersSortedMap]
return makeIterator(() => value, 'Headers',
'key')
}
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's necessary to keep it, because it's a fast-path to avoid shallow cloning.
However, It can be left to avoid getter.


return makeIterator(
() => [...this[kHeadersSortedMap].values()],
() => this[kHeadersSortedMap],
'Headers',
'key'
'key',
0, 1
)
}

values () {
webidl.brandCheck(this, Headers)

if (this[kGuard] === 'immutable') {
const value = this[kHeadersSortedMap]
return makeIterator(() => value, 'Headers',
'value')
}

return makeIterator(
() => [...this[kHeadersSortedMap].values()],
() => this[kHeadersSortedMap],
'Headers',
'value'
'value',
0, 1
)
}

entries () {
webidl.brandCheck(this, Headers)

if (this[kGuard] === 'immutable') {
const value = this[kHeadersSortedMap]
return makeIterator(() => value, 'Headers',
'key+value')
}

return makeIterator(
() => [...this[kHeadersSortedMap].values()],
() => this[kHeadersSortedMap],
Comment on lines 517 to +518
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need to do a shallow clone here.

Copy link
Member

Choose a reason for hiding this comment

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

Actually there is. The headers might change while being iterated?

'Headers',
'key+value'
'key+value',
0, 1
)
}

Expand All @@ -553,7 +538,7 @@ class Headers {
}

for (const [key, value] of this) {
callbackFn.apply(thisArg, [value, key, this])
callbackFn.call(thisArg, value, key, this)
}
}

Expand Down
118 changes: 58 additions & 60 deletions lib/fetch/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -739,19 +739,23 @@ const esIteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbo

/**
* @see https://webidl.spec.whatwg.org/#dfn-iterator-prototype-object
* @param {() => unknown[]} iterator
* @param {() => unknown} iterator
* @param {string} name name of the instance
* @param {'key'|'value'|'key+value'} kind
* @param {string | number} [keyIndex]
* @param {string | number} [valueIndex]
*/
function makeIterator (iterator, name, kind) {
function makeIterator (iterator, name, kind, keyIndex = 0, valueIndex = 1) {
const object = {
index: 0,
kind,
target: iterator
}
// The [[Prototype]] internal slot of an iterator prototype object must be %IteratorPrototype%.
const iteratorObject = Object.create(esIteratorPrototype)
Copy link
Member Author

Choose a reason for hiding this comment

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

Object.create faster than Object.setPrototypeOf.


const i = {
next () {
Object.defineProperty(iteratorObject, 'next', {
value: function next () {
// 1. Let interface be the interface for which the iterator prototype object exists.

// 2. Let thisValue be the this value.
Expand All @@ -763,7 +767,7 @@ function makeIterator (iterator, name, kind) {

// 5. If object is not a default iterator object for interface,
// then throw a TypeError.
if (Object.getPrototypeOf(this) !== i) {
if (Object.getPrototypeOf(this) !== iteratorObject) {
throw new TypeError(
`'next' called on an object that does not implement interface ${name} Iterator.`
)
Expand All @@ -783,68 +787,62 @@ function makeIterator (iterator, name, kind) {
if (index >= len) {
return { value: undefined, done: true }
}

// 11. Let pair be the entry in values at index index.
const pair = values[index]

const { [keyIndex]: key, [valueIndex]: value } = values[index]
// 12. Set object’s index to index + 1.
object.index = index + 1

// 13. Return the iterator result for pair and kind.
return iteratorResult(pair, kind)
// https://webidl.spec.whatwg.org/#iterator-result
// 1. Let result be a value determined by the value of kind:
let result
if (kind === 'key') {
tsctx marked this conversation as resolved.
Show resolved Hide resolved
// 1. Let idlKey be pair’s key.
// 2. Let key be the result of converting idlKey to an
// ECMAScript value.
// 3. result is key.
result = key
} else if (kind === 'value') {
// 1. Let idlValue be pair’s value.
// 2. Let value be the result of converting idlValue to
// an ECMAScript value.
// 3. result is value.
result = value
} else {
// 1. Let idlKey be pair’s key.
// 2. Let idlValue be pair’s value.
// 3. Let key be the result of converting idlKey to an
// ECMAScript value.
// 4. Let value be the result of converting idlValue to
// an ECMAScript value.
// 5. Let array be ! ArrayCreate(2).
// 6. Call ! CreateDataProperty(array, "0", key).
// 7. Call ! CreateDataProperty(array, "1", value).
// 8. result is array.
result = [key, value]
}
// 2. Return CreateIterResultObject(result, false).
return {
value: result,
done: false
}
},
// The class string of an iterator prototype object for a given interface is the
// result of concatenating the identifier of the interface and the string " Iterator".
[Symbol.toStringTag]: `${name} Iterator`
}

// The [[Prototype]] internal slot of an iterator prototype object must be %IteratorPrototype%.
Object.setPrototypeOf(i, esIteratorPrototype)
// esIteratorPrototype needs to be the prototype of i
// which is the prototype of an empty object. Yes, it's confusing.
return Object.setPrototypeOf({}, i)
}
writable: true,
enumerable: true,
configurable: true
})

// https://webidl.spec.whatwg.org/#iterator-result
function iteratorResult (pair, kind) {
let result

// 1. Let result be a value determined by the value of kind:
switch (kind) {
case 'key': {
// 1. Let idlKey be pair’s key.
// 2. Let key be the result of converting idlKey to an
// ECMAScript value.
// 3. result is key.
result = pair[0]
break
}
case 'value': {
// 1. Let idlValue be pair’s value.
// 2. Let value be the result of converting idlValue to
// an ECMAScript value.
// 3. result is value.
result = pair[1]
break
}
case 'key+value': {
// 1. Let idlKey be pair’s key.
// 2. Let idlValue be pair’s value.
// 3. Let key be the result of converting idlKey to an
// ECMAScript value.
// 4. Let value be the result of converting idlValue to
// an ECMAScript value.
// 5. Let array be ! ArrayCreate(2).
// 6. Call ! CreateDataProperty(array, "0", key).
// 7. Call ! CreateDataProperty(array, "1", value).
// 8. result is array.
result = pair
break
}
}
// The class string of an iterator prototype object for a given interface is the
// result of concatenating the identifier of the interface and the string " Iterator".
Object.defineProperty(iteratorObject, Symbol.toStringTag, {
value: `${name} Iterator`,
writable: false,
enumerable: false,
configurable: true
})
Comment on lines +840 to +845
Copy link
Member Author

Choose a reason for hiding this comment

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

writable and enumerable must be set to false.


// 2. Return CreateIterResultObject(result, false).
return { value: result, done: false }
// esIteratorPrototype needs to be the prototype of iteratorObject
// which is the prototype of an empty object. Yes, it's confusing.
return Object.create(iteratorObject)
}

/**
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@
"jest": "^29.0.2",
"jsdom": "^24.0.0",
"jsfuzz": "^1.0.15",
"mitata": "^0.1.6",
"mitata": "^0.1.8",
"mocha": "^10.0.0",
"p-timeout": "^3.2.0",
"pre-commit": "^1.2.2",
Expand Down
Loading