https://github.com/nodejs/node/pull/54474
저번에 이어 이번이 두 번째 기여가 되었다.
(4번의 시도 중 두 번이 merge 되었다)
이번에 수정할 부분은 아래와 같다.
for (const key of ObjectKeys(streamReturningOperators)) {
const op = streamReturningOperators[key];
function fn(...args) {
if (new.target) {
throw new ERR_ILLEGAL_CONSTRUCTOR();
}
return Stream.Readable.from(ReflectApply(op, this, args));
}
ObjectDefineProperty(fn, 'name', { __proto__: null, value: op.name });
ObjectDefineProperty(fn, 'length', { __proto__: null, value: op.length });
ObjectDefineProperty(Stream.Readable.prototype, key, {
__proto__: null,
value: fn,
enumerable: false,
configurable: true,
writable: true,
});
}
for (const key of ObjectKeys(promiseReturningOperators)) {
const op = promiseReturningOperators[key];
function fn(...args) {
if (new.target) {
throw new ERR_ILLEGAL_CONSTRUCTOR();
}
return ReflectApply(op, this, args);
}
ObjectDefineProperty(fn, 'name', { __proto__: null, value: op.name });
ObjectDefineProperty(fn, 'length', { __proto__: null, value: op.length });
ObjectDefineProperty(Stream.Readable.prototype, key, {
__proto__: null,
value: fn,
enumerable: false,
configurable: true,
writable: true,
});
}
이 부분은 stream과 관련된 부분이며 관련 객체를 설정하는 부분이다.
개선점 찾기
위의 코드를 구조적으로 수정할 수 있는 부분은 따로 없어보인다.
primordials를 잘 사용하고 있으며 성능상 크게 문제 되는 부분도 없는 것으로 보인다.
성능을 올릴 마땅한 방법은 없어 보이지만 이 코드에도 수정할 수 있는 지점이 존재한다.
primordials의 문서를 읽어보면 아래와 같은 부분이 있다.
https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration
이 문서에 의하면 for...of의 경우 안전하지 않은 배열 이터레이션이라 한다.
이 부분이 수정할 지점이 될 것이다.
개선 방법
위의 코드를 수정하는 것은 크게 어렵지 않다.
단순히 for...of를 사용하는 대신 Index 기반의 for loop를 만들면 된다.
const streamKeys = ObjectKeys(streamReturningOperators);
for (let i = 0; i < streamKeys.length; i++) {
const key = streamKeys[i];
const op = streamReturningOperators[key];
function fn(...args) {
if (new.target) {
throw new ERR_ILLEGAL_CONSTRUCTOR();
}
return Stream.Readable.from(ReflectApply(op, this, args));
}
ObjectDefineProperty(fn, 'name', { __proto__: null, value: op.name });
ObjectDefineProperty(fn, 'length', { __proto__: null, value: op.length });
ObjectDefineProperty(Stream.Readable.prototype, key, {
__proto__: null,
value: fn,
enumerable: false,
configurable: true,
writable: true,
});
}
const promiseKeys = ObjectKeys(promiseReturningOperators);
for (let i = 0; i < promiseKeys.length; i++) {
const key = promiseKeys[i];
const op = promiseReturningOperators[key];
function fn(...args) {
if (new.target) {
throw new ERR_ILLEGAL_CONSTRUCTOR();
}
return ReflectApply(op, this, args);
}
ObjectDefineProperty(fn, 'name', { __proto__: null, value: op.name });
ObjectDefineProperty(fn, 'length', { __proto__: null, value: op.length });
ObjectDefineProperty(Stream.Readable.prototype, key, {
__proto__: null,
value: fn,
enumerable: false,
configurable: true,
writable: true,
});
}
위와 같이 수정한다면 index를 사용해서 for loop를 사용할 수 있게 된다.
(다른 부분의 코드는 수정하지 않았다)
문서에서 추천하는 방법으로 개선을 했지만 이에 대해 성능상 문제가 없는지 확인해야 한다.
(테스트는 모두 통과한다는 가정)
벤치마크
confidence improvement accuracy (*) (**) (***)
streams/compose.js n=1000 -0.19 % ±0.55% ±0.73% ±0.95%
streams/creation.js kind='duplex' n=50000000 -0.66 % ±3.62% ±4.82% ±6.28%
streams/creation.js kind='readable' n=50000000 -0.43 % ±5.08% ±6.78% ±8.86%
streams/creation.js kind='transform' n=50000000 0.30 % ±6.37% ±8.48% ±11.05%
streams/creation.js kind='writable' n=50000000 -2.93 % ±5.08% ±6.81% ±8.96%
streams/destroy.js kind='duplex' n=1000000 1.12 % ±1.55% ±2.07% ±2.72%
streams/destroy.js kind='readable' n=1000000 * 1.50 % ±1.47% ±1.97% ±2.59%
streams/destroy.js kind='transform' n=1000000 0.63 % ±1.10% ±1.47% ±1.91%
streams/destroy.js kind='writable' n=1000000 0.75 % ±1.79% ±2.38% ±3.10%
streams/pipe-object-mode.js n=5000000 -1.15 % ±2.70% ±3.63% ±4.82%
streams/pipe.js n=5000000 0.73 % ±1.18% ±1.57% ±2.05%
streams/readable-async-iterator.js sync='no' n=100000 * -0.69 % ±0.54% ±0.72% ±0.94%
streams/readable-async-iterator.js sync='yes' n=100000 -0.89 % ±1.40% ±1.87% ±2.45%
streams/readable-bigread.js n=1000 -0.59 % ±0.68% ±0.90% ±1.17%
streams/readable-bigunevenread.js n=1000 -0.04 % ±0.60% ±0.80% ±1.04%
streams/readable-boundaryread.js type='buffer' n=2000 0.18 % ±1.02% ±1.36% ±1.77%
streams/readable-boundaryread.js type='string' n=2000 -0.64 % ±0.89% ±1.19% ±1.54%
streams/readable-from.js type='array' n=10000000 1.66 % ±7.10% ±9.45% ±12.31%
streams/readable-from.js type='async-generator' n=10000000 0.27 % ±1.07% ±1.42% ±1.85%
streams/readable-from.js type='sync-generator-with-async-values' n=10000000 -1.30 % ±1.72% ±2.28% ±2.98%
streams/readable-from.js type='sync-generator-with-sync-values' n=10000000 -0.29 % ±1.54% ±2.05% ±2.67%
streams/readable-readall.js n=5000 -0.20 % ±0.36% ±0.49% ±0.63%
streams/readable-uint8array.js kind='encoding' n=1000000 *** 2.43 % ±0.79% ±1.05% ±1.37%
streams/readable-uint8array.js kind='read' n=1000000 -0.96 % ±1.77% ±2.35% ±3.06%
streams/readable-unevenread.js n=1000 0.04 % ±0.51% ±0.67% ±0.88%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000 -0.30 % ±0.89% ±1.19% ±1.55%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000 * -0.70 % ±0.68% ±0.90% ±1.18%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000 ** -0.71 % ±0.53% ±0.70% ±0.91%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000 -0.42 % ±0.82% ±1.09% ±1.42%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000 0.65 % ±1.28% ±1.71% ±2.22%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000 1.10 % ±2.19% ±2.94% ±3.90%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000 * -0.89 % ±0.83% ±1.10% ±1.43%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000 -0.14 % ±1.42% ±1.89% ±2.46%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000 -0.04 % ±0.98% ±1.31% ±1.70%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000 * -0.56 % ±0.53% ±0.71% ±0.92%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000 0.24 % ±0.87% ±1.17% ±1.53%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000 0.27 % ±0.73% ±0.98% ±1.27%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000 -0.03 % ±2.01% ±2.69% ±3.51%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000 -0.36 % ±1.21% ±1.62% ±2.12%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000 -0.26 % ±0.94% ±1.26% ±1.65%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000 -0.30 % ±0.72% ±0.96% ±1.25%
streams/writable-uint8array.js kind='object-mode' n=50000000 -0.78 % ±1.82% ±2.45% ±3.23%
streams/writable-uint8array.js kind='write' n=50000000 -2.21 % ±2.45% ±3.30% ±4.36%
streams/writable-uint8array.js kind='writev' n=50000000 -0.14 % ±1.22% ±1.62% ±2.12%
stream과 관련된 부분의 모든 벤치마크를 돌려보면 위와 같은 결과가 나온다.
위의 경우는 개인 로컬 환경에서 수행한 벤치마크이며 Node.js의 Jenkins CI를 사용한 벤치마크 결과는 아래와 같다.
confidence improvement accuracy (*) (**) (***)
streams/compose.js n=1000 -0.08 % ±0.54% ±0.71% ±0.93%
streams/creation.js kind='duplex' n=50000000 1.11 % ±2.20% ±2.93% ±3.81%
streams/creation.js kind='readable' n=50000000 1.09 % ±1.39% ±1.85% ±2.41%
streams/creation.js kind='transform' n=50000000 0.75 % ±2.55% ±3.40% ±4.43%
streams/creation.js kind='writable' n=50000000 -0.77 % ±1.35% ±1.80% ±2.34%
streams/destroy.js kind='duplex' n=1000000 -0.06 % ±0.53% ±0.71% ±0.92%
streams/destroy.js kind='readable' n=1000000 0.41 % ±0.74% ±0.98% ±1.27%
streams/destroy.js kind='transform' n=1000000 -0.04 % ±0.44% ±0.58% ±0.76%
streams/destroy.js kind='writable' n=1000000 -0.50 % ±0.87% ±1.16% ±1.51%
streams/pipe-object-mode.js n=5000000 -0.20 % ±0.28% ±0.38% ±0.50%
streams/pipe.js n=5000000 ** 0.39 % ±0.28% ±0.37% ±0.49%
streams/readable-async-iterator.js sync='no' n=100000 -0.19 % ±0.61% ±0.82% ±1.06%
streams/readable-async-iterator.js sync='yes' n=100000 -0.38 % ±0.62% ±0.83% ±1.08%
streams/readable-bigread.js n=1000 -0.16 % ±0.73% ±0.97% ±1.28%
streams/readable-bigunevenread.js n=1000 0.11 % ±0.35% ±0.46% ±0.61%
streams/readable-boundaryread.js type='buffer' n=2000 -0.40 % ±0.70% ±0.94% ±1.22%
streams/readable-boundaryread.js type='string' n=2000 0.33 % ±0.68% ±0.90% ±1.18%
streams/readable-from.js type='array' n=10000000 0.39 % ±2.50% ±3.32% ±4.32%
streams/readable-from.js type='async-generator' n=10000000 -0.60 % ±0.73% ±0.97% ±1.26%
streams/readable-from.js type='sync-generator-with-async-values' n=10000000 0.22 % ±0.35% ±0.47% ±0.61%
streams/readable-from.js type='sync-generator-with-sync-values' n=10000000 -0.06 % ±0.20% ±0.26% ±0.34%
streams/readable-readall.js n=5000 2.01 % ±2.23% ±2.97% ±3.86%
streams/readable-uint8array.js kind='encoding' n=1000000 0.01 % ±0.55% ±0.73% ±0.95%
streams/readable-uint8array.js kind='read' n=1000000 0.45 % ±2.24% ±2.98% ±3.88%
streams/readable-unevenread.js n=1000 -0.08 % ±0.27% ±0.36% ±0.47%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=100000 0.15 % ±0.94% ±1.24% ±1.62%
streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=100000 -0.32 % ±1.14% ±1.52% ±1.98%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=100000 0.09 % ±0.51% ±0.68% ±0.88%
streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=100000 -0.01 % ±1.21% ±1.60% ±2.09%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=100000 -0.25 % ±1.40% ±1.86% ±2.42%
streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=100000 0.10 % ±0.80% ±1.07% ±1.39%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=100000 0.53 % ±0.60% ±0.80% ±1.04%
streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=100000 -0.64 % ±1.03% ±1.37% ±1.78%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=100000 0.72 % ±1.66% ±2.23% ±2.95%
streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=100000 -0.57 % ±1.19% ±1.58% ±2.06%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=100000 -0.57 % ±0.75% ±1.00% ±1.30%
streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=100000 -0.11 % ±1.20% ±1.61% ±2.11%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=100000 -0.93 % ±1.22% ±1.63% ±2.15%
streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=100000 -0.49 % ±0.92% ±1.22% ±1.59%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=100000 0.17 % ±0.54% ±0.72% ±0.95%
streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=100000 -0.47 % ±0.89% ±1.18% ±1.54%
streams/writable-uint8array.js kind='object-mode' n=50000000 0.00 % ±0.04% ±0.06% ±0.08%
streams/writable-uint8array.js kind='write' n=50000000 0.09 % ±0.83% ±1.10% ±1.44%
streams/writable-uint8array.js kind='writev' n=50000000 0.52 % ±0.95% ±1.26% ±1.64%
로컬의 경우 confidence가 높은 벤치마크들이 다수 존재하지만 Jenkins의 경우는 하나의 케이스만 confidence 케이스로 분류되었다.
(환경에 따라 또는 벤치마크 수행 시점마다 결과가 다르게 나오는 경우가 있는 것으로 보인다)
Jenkins를 기존으로 본다면 유의할 만한 성능 감소는 보이지 않았으며 유의미한 차이의 경우도 성능 저하가 보이지는 않았다.
이를 바탕으로 성능에 하락이 없으며 Node.js의 컨벤션을 따르도록 수정했다는 것을 보여주며 PR을 열게 되었다.
Unsafe Array Iteration?
여기서 주목해야 하는 점은 Unsafe라는 것이다.
for...of 문은 어떤 이유에서 index 기반의 for loop보다 안전하지 않다고 하는 것일까?
for...of 문은 이터러블을 순회하면서 이터러블 내부의 요소를 변수에 할당한다.
이는 내부적으로 이터레이터의 next 문을 호출하여 이터러블을 순회하며 next 메소드가 반환한 이터레이터 결과 객체의 value 프로퍼티의 값을 for...of 문의 변수에 할당한다.
또한, 이터레이터 결과 객체의 done 프로퍼티가 true인 경우 이터러블의 순회를 중단하게 된다.
Iteration
이터레이션은 ES6에 도입되었으며 순회 가능한 자료구조를 만들기 위한 규칙이다.
이를 다시 생각해보면 ES6 도입 이전에는 순회 가능한 자료구조들은 통일된 규격이 없었으며 for, for...in, forEach등을 사용하여 순회하고 있었다는 것이다.
ES6에서는 이러한 순회 가능 자료구조들을 이터레이션 프로토콜을 준수하는 이터러블로 통일하여 for...of, 스프레드 문법, 배열 구조분해 할당의 대상으로 사용할 수 있도록 한 것이다.
여기서 이터러블 프로토콜과 이터레이터 프로토콜이라는 개념이 나오게 된다.
- 이터러블 프로토콜: Symbol.iterator를 프로퍼티의 키로 사용한 메소드를 직접 구현하거나 프로토타입 체인을 통해 상속받은 Symbol.iterator 메소드를 호출하는 경우 이터레이터 프로토콜을 준수한 이터레이터를 반환하게 되며 이러한 프로토콜을 따르는 객체를 이터러블이라 한다.
- 이터레이터 프로토콜: 이터레이터의 Symbol.iterator 메소드를 호출하면 이터레이터 프로토콜을 준수한 이터레이터를 반환한다.
- 이터러블: 이터러블 프로토콜을 준수한 객체.
결론적으로 이터러블 프로토콜은 이터러블 객체가 따라야 할 규칙을 정의한 것이며, 이터레이터 프로토콜의 경우 이터레이터가 따라야 할 규칙을 정의한 것이다.
다시 말해, 이터러블 객체의 Symbol.iterator 메소드를 호출하면 이터레이터 객체가 생성되는 것이며 이터레이터 객체의 경우는 next메소드와 결과 객체를 갖게 되는 것이다.
따라서, 어떤 객체가 이터러블 프로토콜과 이터레이터 프로토콜을 제대로 따르고 있다면 이 객체는 Symbol.iterator 프로퍼티를 갖고 있으며(이터러블 프로토콜) Symbol.iterator는 next 메소드와 done, value를 포함한 결과 객체를 반환할 것이다(이터레이터 프로토콜).
일반 배열을 예시로 보면 다음과 같이 Symbol.iterator 프로퍼티를 갖고 있는 것을 볼 수 있다.
이를 바탕으로 Symbol.iterator를 보면 다음과 같은 결과를 얻을 수 있다.
앞에서 봤던 내용을 바탕으로 이터러블 프로토콜을 준수한다면 Symbol.iterator는 메소드이므로 호출이 가능하다.
(이터러블 프로토콜을 준수하기 위해서는 Symbol.iterator를 프로퍼티 키로 사용한 메소드가 있어야 하며 이 메소드를 호출하면 이터레이터를 반환해야 한다. 위의 경우 b 변수에는 이터레이터가 할당될 것이다)
이를 호출한다면 위와 같은 결과를 얻을 수 있으며 next 메소드를 호출하는 경우 value와 done을 포함한 결과 객체를 반환하는 것을 볼 수 있다.
(이터레이터 프로토콜을 준수하기 위해서는 next 메소드를 갖고 있으며 결과 객체를 반환해야 한다)
또한, done이 true가 된 경우는 순회가 종료된 것으로 판단하며 value의 경우는 존재하지 않기 때문에 undefined가 되는 것을 확인할 수 있다.
이를 바탕으로 배열은 이터러블이라는 결론을 내릴 수 있을 것이다.
결론
다시 본론으로 돌아와서 for...of가 안전하지 않은 이유에 대해 알아보자.
앞에서 for...of는 ES6에서 도입된 이터레이션 프로토콜을 활용한 문법이라는 부분을 언급했다.
배열의 경우 Array.prototype의 Symbol.iterator 메소드를 상속받는 이터러블이기 때문에 for...of로 순회가 가능한 것이다.
이를 안전하지 않다고 하는 것은 primordials를 사용하는 이유와 같은 이유일 것으로 보인다.
결국 프로토타입 내부에 Iterator가 정의되어 있으며 이는 사용자 측에서 수정이 가능하기 때문이다.
// User-land
Array.prototype[Symbol.iterator] = () => ({
next: () => ({ done: true }),
});
// Core
let forOfLoopBlockExecuted = false;
let forLoopBlockExecuted = false;
const array = [1, 2, 3];
for (const item of array) {
forOfLoopBlockExecuted = true;
}
for (let i = 0; i < array.length; i++) {
forLoopBlockExecuted = true;
}
console.log(forOfLoopBlockExecuted); // false
console.log(forLoopBlockExecuted); // true
이런 예시가 있을 때, 이터레이터의 경우 done이라는 값을 갖는 결과 객체를 반환하며 이를 바탕으로 순회의 종료 여부를 판단한다고 했다.
만약, 사용자가 Array 객체의 프로토타입에 직접 접근하여 Symbol.iterator의 행위를 수정하는 경우를 가정해보면 위와 같은 문제가 발생한다.
for...of를 사용하게 된다면 해당 loop가 시작되자 마자 done이 true이므로 loop가 종료된다.
그와 반대로 index를 사용하는 전통적인 방식의 경우 이터레이터에 의존하지 않기 때문에 prototype pollution에 보다 더 안전하다는 결론을 내릴 수 있다.
PR
문서에 언급한 대로 수정하는 간단한 작업이었기에 별다른 코멘트는 없었다.
벤치마크를 통해 성능 저하가 없음을 보여줬기 때문에 더 언급할 부분은 없다고 생각했다.
최종적으로는 아래와 같이 간단하게 위 문서에 대한 Refs를 걸고 벤치마크 결과와 함께 PR을 올리게 되었다.
stream과 관련된 부분이라 그런지 많은 분들이 검토하고 가신 것 같다.
결과
별 다른 문제 없이 merge 되었고 이것으로 두 번째 기여를 할 수 있었다.
성능의 개선이 아니라 아쉽긴 하지만 이번 경험을 바탕으로 prototype pollution에 대한 Node.js 팀의 생각을 볼 수 있었다.
또한, 이터레이터에 대해서 조금 더 deep dive 할 수 있었다.
새롭게 도입된 기술들에 대해 편리함을 느끼면서도 그 내부에는 어떤 개념이 포함되어 있으며 기존 방식과의 차이점이 무엇인지, 어떤 잠재적 문제가 있을지 생각해볼 수 있는 기회였다고 생각한다.
'개발 > 개발과정' 카테고리의 다른 글
[Node.js 기여하기] 3. path 모듈 resolve 함수 성능 올리기 (0) | 2024.09.18 |
---|---|
[Node.js 기여하기] 1. path 모듈 join 함수 성능 올리기 (1) | 2024.08.16 |
[import-visualizer] 10. 개발 회고 (0) | 2024.06.14 |
[import-visualizer] 9. 상대경로 문제와 스택 오버플로우 (0) | 2024.06.14 |
[import-visualizer] 8. tsc의 한계 및 배포 (0) | 2024.06.14 |