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: always use the same prototype Iterator #2743

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
27 changes: 7 additions & 20 deletions lib/fetch/formdata.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict'

const { isBlobLike, makeIterator } = require('./util')
const { isBlobLike, createIterator } = require('./util')
const { kState } = require('./symbols')
const { kEnumerableProperty } = require('../core/util')
const { File: UndiciFile, FileLike, isFileLike } = require('./file')
Expand All @@ -10,6 +10,8 @@ const { File: NativeFile } = require('node:buffer')
/** @type {globalThis['File']} */
const File = NativeFile ?? UndiciFile

const makeIterator = createIterator('FormData', kState, 'name', 'value')

// https://xhr.spec.whatwg.org/#formdata
class FormData {
constructor (form) {
Expand Down Expand Up @@ -158,34 +160,19 @@ class FormData {
entries () {
webidl.brandCheck(this, FormData)

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

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

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

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

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

/**
Expand All @@ -203,7 +190,7 @@ class FormData {
)
}

for (const [key, value] of this) {
for (const [key, value] of makeIterator(this, 'key+value')) {
callbackFn.call(thisArg, value, key, this)
}
}
Expand Down
28 changes: 8 additions & 20 deletions lib/fetch/headers.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// https://github.com/Ethan-Arrowood/undici-fetch
// https://github.com/Ethan-Arrowood/undici-fetch

'use strict'

const { kHeadersList, kConstruct } = require('../core/symbols')
const { kGuard } = require('./symbols')
const { kEnumerableProperty } = require('../core/util')
const {
makeIterator,
createIterator,
isValidHeaderName,
isValidHeaderValue
} = require('./util')
Expand All @@ -16,6 +17,8 @@ const assert = require('node:assert')
const kHeadersMap = Symbol('headers map')
const kHeadersSortedMap = Symbol('headers map sorted')

const makeIterator = createIterator('Headers', kHeadersSortedMap, 0, 1)

/**
* @param {number} code
*/
Expand Down Expand Up @@ -507,34 +510,19 @@ class Headers {
keys () {
webidl.brandCheck(this, Headers)

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

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

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

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

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

/**
Expand All @@ -552,7 +540,7 @@ class Headers {
)
}

for (const [key, value] of this) {
for (const [key, value] of makeIterator(this, 'key+value')) {
callbackFn.call(thisArg, value, key, this)
}
}
Expand Down
46 changes: 30 additions & 16 deletions lib/fetch/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -739,18 +739,14 @@ const esIteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbo

/**
* @see https://webidl.spec.whatwg.org/#dfn-iterator-prototype-object
* @param {() => unknown} iterator
* @param {string} name name of the instance
* @param {'key'|'value'|'key+value'} kind
* @param {symbol} kInternalIterator
* @param {string | number} [keyIndex]
* @param {string | number} [valueIndex]
*/
function makeIterator (iterator, name, kind, keyIndex = 0, valueIndex = 1) {
const object = {
index: 0,
kind,
target: iterator
}
function createIterator (name, kInternalIterator, keyIndex = 0, valueIndex = 1) {
const kInternalObject = Symbol('internal Object')
tsctx marked this conversation as resolved.
Show resolved Hide resolved

// The [[Prototype]] internal slot of an iterator prototype object must be %IteratorPrototype%.
const iteratorObject = Object.create(esIteratorPrototype)

Expand All @@ -767,17 +763,18 @@ function makeIterator (iterator, name, kind, keyIndex = 0, valueIndex = 1) {

// 5. If object is not a default iterator object for interface,
// then throw a TypeError.
if (Object.getPrototypeOf(this) !== iteratorObject) {
if (typeof this !== 'object' || this === null || !(kInternalObject in this)) {
throw new TypeError(
`'next' called on an object that does not implement interface ${name} Iterator.`
)
}
const state = this[kInternalObject]
tsctx marked this conversation as resolved.
Show resolved Hide resolved

// 6. Let index be object’s index.
// 7. Let kind be object’s kind.
// 8. Let values be object’s target's value pairs to iterate over.
const { index, kind, target } = object
const values = target()
const { index, kind, target } = state
const values = target[kInternalIterator]

// 9. Let len be the length of values.
const len = values.length
Expand All @@ -790,7 +787,7 @@ function makeIterator (iterator, name, kind, keyIndex = 0, valueIndex = 1) {
// 11. Let pair be the entry in values at index index.
const { [keyIndex]: key, [valueIndex]: value } = values[index]
// 12. Set object’s index to index + 1.
object.index = index + 1
state.index = index + 1
// 13. Return the iterator result for pair and kind.
// https://webidl.spec.whatwg.org/#iterator-result
// 1. Let result be a value determined by the value of kind:
Expand Down Expand Up @@ -844,9 +841,26 @@ function makeIterator (iterator, name, kind, keyIndex = 0, valueIndex = 1) {
configurable: true
})

// esIteratorPrototype needs to be the prototype of iteratorObject
// which is the prototype of an empty object. Yes, it's confusing.
return Object.create(iteratorObject)
/**
* @param {unknown} target
* @param {'key'|'value'|'key+value'} kind
*/
return function (target, kind) {
// esIteratorPrototype needs to be the prototype of iteratorObject
// which is the prototype of an empty object. Yes, it's confusing.
const iterator = Object.create(iteratorObject)
Object.defineProperty(iterator, kInternalObject, {
value: {
target,
kind,
index: 0
},
writable: false,
enumerable: false,
configurable: true
})
return iterator
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we somehow avoid Object.create and Object.defineProperty?

Maybe using class?

Usually those functions are slow.

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 agree. Let's optimize in this way. tsctx/webidl-dfn-iterator-prototype-object@3eee31d

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. 19a7d8d

Copy link
Contributor

Choose a reason for hiding this comment

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

nice

}

/**
Expand Down Expand Up @@ -1365,7 +1379,7 @@ module.exports = {
sameOrigin,
normalizeMethod,
serializeJavascriptValueToJSONString,
makeIterator,
createIterator,
isValidHeaderName,
isValidHeaderValue,
isErrorLike,
Expand Down
Loading