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

Feat/pull mplex #1

Closed
wants to merge 39 commits into from
Closed

Feat/pull mplex #1

wants to merge 39 commits into from

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Apr 15, 2018

UPDATE: This is now ready for review.

TODO:

  • Performance
    • pool channel objects (should come next, but not critical for first release)

Bellow are the accompanying PRs:


The current implementation takes around ~15mins more running the mega stress tests defined in interface-stream-muxer.

Here is the outline:

New:

screen shot 2018-04-21 at 11 08 05 am

Old:

screen shot 2018-04-21 at 6 32 53 pm


The issue is here - https://github.com/libp2p/pull-mplex/blob/850bbc52c33038f2cdeff40e83afbcf4823b3895/src/coder.js#L85...L89. I need to rework this part to use a preallocated buffer, instead of allocating it every time.

@daviddias
Copy link
Member

@dryajov make this a PR to libp2p-mplex, not a new module.

@dryajov
Copy link
Member Author

dryajov commented Apr 15, 2018

This module is implemented standalone so it can be consumed outside of libp2p, libp2p-mplex has been modified accordingly to consume it. The main idea is that this can be consumed by other projects.

@dryajov
Copy link
Member Author

dryajov commented Apr 15, 2018

BTW, the same approach was taken with the go implementation - https://github.com/libp2p/go-mplex

@dryajov
Copy link
Member Author

dryajov commented Apr 28, 2018

I was able to shave off ~15 mins of the new implementations running the mega stress tests, the perf is now about the same (or a little better ;) ) than the stream based implementation.

screen shot 2018-04-27 at 7 13 30 pm

Archive.zip

The attached zip contains a heap snapshot and a perf log taken with node --prof --nologfile_per_isolate --logfile=xxxxxx.log --log-timer-events /private/var/folders/_r/6c6jf6m10kb3v9kt45qspw4w0000gn/T/v8profilerProxy.js. The one thing that still irks me a bit is the excessive (compared to the prev implementation) GC time. I think we can improve that a lot by pooling the Channel objects and reusing them, rather than creating new ones every time.

Here is a screenshot from the perf log graph - GC is seems to take ~13% of the total time. (These graphs where generated using the attached logs, and webstorm perf and heap analyzers).

screen shot 2018-04-27 at 7 28 07 pm

screen shot 2018-04-27 at 7 27 54 pm

@ghost ghost assigned dryajov May 5, 2018
@ghost ghost added the status/in-progress In progress label May 5, 2018
@dryajov dryajov changed the title [WIP] Feat/pull mplex Feat/pull mplex May 9, 2018
@dryajov dryajov requested review from pgte, jacobheun and daviddias May 9, 2018 18:03
@dryajov
Copy link
Member Author

dryajov commented May 10, 2018

I'd love to get some feedback from @Stebalien as well.

@dryajov dryajov closed this May 10, 2018
@dryajov dryajov force-pushed the feat/pull-mplex branch from 1c2362f to 96e89d8 Compare May 10, 2018 18:33
@ghost ghost removed the status/in-progress In progress label May 10, 2018
@dryajov dryajov deleted the feat/pull-mplex branch May 10, 2018 18:37
@dryajov dryajov restored the feat/pull-mplex branch May 10, 2018 18:48
@dryajov dryajov deleted the feat/pull-mplex branch May 10, 2018 19:04
@dryajov dryajov mentioned this pull request May 10, 2018
2 tasks
@dryajov
Copy link
Member Author

dryajov commented May 10, 2018

This PR got horked badly, please use #2 for further dev/reviews.

This was referenced May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants