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

Optimize data properties #17

Merged
merged 3 commits into from
Mar 16, 2019
Merged

Optimize data properties #17

merged 3 commits into from
Mar 16, 2019

Conversation

MattiasBuelens
Copy link
Owner

@MattiasBuelens MattiasBuelens commented Mar 16, 2019

In #16, I noticed that Stardazed Streams were more than 2x faster than this polyfill. This sparked my interest, so I set out to find where this polyfill was spending the extra time.

I set up a benchmark suite using Benchmark.js to compare the performance of this polyfill and Stardazed's implementation on the code snippet from #15. Then, I used flamebearer to create the following flame graphs:

web-streams-polyfill
flamegraph-polyfill
@stardazed/streams
flamegraph-stardazed

I'm not an expert, but it looks like the polyfill's reader.read() spends a lot of time in some sort of built-in Node function.

I went to compare the ReadableStreamDefaultReaderRead implementation of web-streams-polyfill and @stardazed/streams, and I only found one difference inside ReadableStreamCreateReadResult. The polyfill uses Object.defineProperty to set value and done:

  Object.defineProperty(obj, 'value', { value, enumerable: true, writable: true, configurable: true });
  Object.defineProperty(obj, 'done', { value: done, enumerable: true, writable: true, configurable: true });

While Stardazed sets those with a regular property assignment:

  result.value = value;
  result.done = done;

Indeed, that fixes the performance issue! 🎉
flamegraph-polyfill-fixed

Benchmark results before:

> node --prof index.js

web-streams-polyfill testCount(3545) x 205 ops/sec ±1.20% (82 runs sampled) (period: 4.88ms)
web-streams-polyfill testCount(7090) x 102 ops/sec ±0.73% (76 runs sampled) (period: 9.82ms)
web-streams-polyfill testCount(14180) x 48.69 ops/sec ±2.41% (74 runs sampled) (period: 20.54ms)
web-streams-polyfill testCount(28360) x 22.74 ops/sec ±1.54% (56 runs sampled) (period: 43.97ms)
web-streams-polyfill testCount(56720) x 10.68 ops/sec ±0.92% (53 runs sampled) (period: 93.60ms)
web-streams-polyfill testCount(113440) x 5.03 ops/sec ±0.77% (28 runs sampled) (period: 198.78ms)
@stardazed/streams testCount(3545) x 835 ops/sec ±1.55% (84 runs sampled) (period: 1.20ms)
@stardazed/streams testCount(7090) x 392 ops/sec ±1.89% (80 runs sampled) (period: 2.55ms)
@stardazed/streams testCount(14180) x 174 ops/sec ±2.63% (78 runs sampled) (period: 5.76ms)
@stardazed/streams testCount(28360) x 73.70 ops/sec ±2.64% (75 runs sampled) (period: 13.57ms)
@stardazed/streams testCount(56720) x 22.85 ops/sec ±5.68% (55 runs sampled) (period: 43.77ms)
@stardazed/streams testCount(113440) x 10.51 ops/sec ±0.91% (52 runs sampled) (period: 95.12ms)
Done

Process finished with exit code 0

After:

> node --prof index.js

web-streams-polyfill testCount(3545) x 742 ops/sec ±2.47% (79 runs sampled) (period: 1.35ms)
web-streams-polyfill testCount(7090) x 362 ops/sec ±1.06% (80 runs sampled) (period: 2.76ms)
web-streams-polyfill testCount(14180) x 167 ops/sec ±1.25% (75 runs sampled) (period: 6.00ms)
web-streams-polyfill testCount(28360) x 73.14 ops/sec ±0.43% (81 runs sampled) (period: 13.67ms)
web-streams-polyfill testCount(56720) x 23.82 ops/sec ±4.17% (57 runs sampled) (period: 41.97ms)
web-streams-polyfill testCount(113440) x 11.08 ops/sec ±0.95% (54 runs sampled) (period: 90.26ms)
@stardazed/streams testCount(3545) x 819 ops/sec ±1.77% (83 runs sampled) (period: 1.22ms)
@stardazed/streams testCount(7090) x 373 ops/sec ±0.51% (79 runs sampled) (period: 2.68ms)
@stardazed/streams testCount(14180) x 167 ops/sec ±1.84% (82 runs sampled) (period: 5.99ms)
@stardazed/streams testCount(28360) x 71.80 ops/sec ±2.17% (80 runs sampled) (period: 13.93ms)
@stardazed/streams testCount(56720) x 21.08 ops/sec ±5.80% (52 runs sampled) (period: 47.43ms)
@stardazed/streams testCount(113440) x 9.35 ops/sec ±0.62% (47 runs sampled) (period: 106.98ms)
Done

Process finished with exit code 0

@MattiasBuelens MattiasBuelens merged commit e9f47c1 into master Mar 16, 2019
@MattiasBuelens MattiasBuelens deleted the optimize-data-properties branch March 16, 2019 23:18
@MattiasBuelens MattiasBuelens added this to the v2.0.2 milestone Mar 17, 2019
@MattiasBuelens MattiasBuelens added domain: performance Issues related to slow performance or high memory usage enhancement labels Mar 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: performance Issues related to slow performance or high memory usage enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant