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

Remove chunker channel #1973

Merged
merged 2 commits into from
Dec 2, 2015
Merged

Remove chunker channel #1973

merged 2 commits into from
Dec 2, 2015

Conversation

rht
Copy link
Contributor

@rht rht commented Nov 16, 2015

PR to keep track and measure the time and memory perf of ipfs add with channel iterator replaced with chunker object. Perhaps have to wait until go2.x when channel iterator becomes about as fast as counter object.

Ran perf test on lots of 1MB files, ipfs add is about O(rsync) (faster than git add?)

Edit: this plot below is misleading (the add operation is run under a goroutine, and without outputDagnode checking, the client exits long before the (ephemeral) daemon has finished the process), updated plot is in my last comment.
We say that people condemn a man to death and then we say the Law condemns him to death. "Although the Jury can pardon (acquit?) him, the Law can't." (This may mean the Law can't take bribes, etc) The idea of something super-strict, something stricter than any judge can be, super-rigidity. The point being, you are inclined to ask: "Do we have a picture of something more rigorous?" Hardly. But we are inclined to express ourselves in the form of a superlative.

@jbenet jbenet added the status/in-progress In progress label Nov 16, 2015
@whyrusleeping
Copy link
Member

@rht this is wonderful.

@whyrusleeping
Copy link
Member

@rht have you tried things with the changes in #1965?

@rht
Copy link
Contributor Author

rht commented Nov 16, 2015

I did, but #1965's memorydagservice optimizes mainly InsertNodeAtPath.
I already saw similar memory profile (i.e. plateau at ~55MB) from that PR when intermediate roots are removed (no-insertnodeatpath).

@jbenet
Copy link
Member

jbenet commented Nov 17, 2015

@rht this is great!! ping me here when i should CR

@rht rht force-pushed the no-chunk-channel branch from 44d005c to 2a5b06b Compare November 18, 2015 14:45
@rht
Copy link
Contributor Author

rht commented Nov 18, 2015

Here is the most recent plot (no-sync + no-insertnodeatpath + no-chunk-channel, each file 1MB).
(to make it simple, removing outputDagnode or not doesn't actually affect the perf, so it is actually slower than rsync)
quiet

@rht rht added the RFCR label Nov 18, 2015
@rht rht force-pushed the no-chunk-channel branch 2 times, most recently from 8603a7d to 3f21b8a Compare November 18, 2015 16:15
@rht rht changed the title Remove chunker channel [WIP] Remove chunker channel Nov 18, 2015
@rht
Copy link
Contributor Author

rht commented Nov 19, 2015

@whyrusleeping if you can confirm the perf of this branch:

  • no-sync (~55(2) s)
  • no-sync + no-insertnodeatpath (~49(2) s)
  • no-sync + no-insertnodeatpath + no-chunk-channel (?)

@rht rht added the topic/perf Performance label Nov 22, 2015
@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

@rht this LGTM -- @whyrusleeping LGTU?

@whyrusleeping
Copy link
Member

yeah, it does. Lets get it rebased first though

rht added 2 commits December 1, 2015 23:04
License: MIT
Signed-off-by: rht <rhtbot@gmail.com>
License: MIT
Signed-off-by: rht <rhtbot@gmail.com>
@rht rht force-pushed the no-chunk-channel branch from 3f21b8a to e359ba1 Compare December 1, 2015 16:06
@rht
Copy link
Contributor Author

rht commented Dec 1, 2015

Rebased.

@rht
Copy link
Contributor Author

rht commented Dec 1, 2015

It is likely the commit that causes random os sharness fails is not in dev0.4.0.
And the bisect in master can be investigated more effectively by someone with an osx.

jbenet added a commit that referenced this pull request Dec 2, 2015
@jbenet jbenet merged commit 63a8e75 into ipfs:dev0.4.0 Dec 2, 2015
@jbenet jbenet removed the status/in-progress In progress label Dec 2, 2015
@jbenet
Copy link
Member

jbenet commented Dec 2, 2015

Thanks @rht !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/perf Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants