Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

fix: reduce async iterator loops per package in _createSink #224

Merged
merged 3 commits into from
Nov 25, 2022

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented Oct 14, 2022

Javascript abstractions are not free. While using pipe here looks nice, it adds a non-neglible cost allocating Promises for each extra for await () iteration.

This PR merges sink pipe components in a single iteration

@achingbrain achingbrain self-requested a review October 18, 2022 15:42
@mpetrunic mpetrunic added the need/maintainer-input Needs input from the current maintainer(s) label Nov 8, 2022
@p-shahi p-shahi added the P2 Medium: Good to have, but can wait until someone steps up label Nov 22, 2022
@achingbrain achingbrain added need/author-input Needs input from the original author and removed need/maintainer-input Needs input from the current maintainer(s) labels Nov 23, 2022
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

I'm not seeing a huge amount of difference in the benchmarks for this repo with the change in this PR applied.

I ran the benchmark 5x times for each branch and took the best result and they look similar enough to me that I'd err on the side of code readability here:

master:

% node benchmark/send-and-receive.js
send and receive x 210,311 ops/sec ±0.19% (98 runs sampled)
Fastest is send and receive

reduce-loops-sink:

% node benchmark/send-and-receive.js
send and receive x 210,287 ops/sec ±0.16% (100 runs sampled)
Fastest is send and receive

Do you have a benchmark that shows a performance improvement with this change?

@dapplion
Copy link
Contributor Author

That branch proves a non-zero cost of async iterables. The goal is progressively reduce the count of async iterables calls or total promises allocated per paquet progressively through the entire libp2p stack.

@achingbrain achingbrain changed the title Reduce async iterator loops per package in _createSink fix: reduce async iterator loops per package in _createSink Nov 24, 2022
@achingbrain achingbrain merged commit e2a32ad into libp2p:master Nov 25, 2022
github-actions bot pushed a commit that referenced this pull request Nov 25, 2022
## [7.0.6](v7.0.5...v7.0.6) (2022-11-25)

### Bug Fixes

* reduce async iterator loops per package in _createSink ([#224](#224)) ([e2a32ad](e2a32ad)), closes [/github.com/libp2p/js-libp2p/issues/1420#issuecomment-1273272662](https://github.com/libp2p//github.com/libp2p/js-libp2p/issues/1420/issues/issuecomment-1273272662)
@github-actions
Copy link

🎉 This PR is included in version 7.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dapplion dapplion deleted the reduce-loops-sink branch December 5, 2022 03:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/author-input Needs input from the original author P2 Medium: Good to have, but can wait until someone steps up released
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants