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

Performance optimizations and memory management #221

Merged
merged 20 commits into from
Nov 2, 2014
Merged

Performance optimizations and memory management #221

merged 20 commits into from
Nov 2, 2014

Conversation

whyrusleeping
Copy link
Member

Added some benchmarks to secure channel and played around with improving its performance, also started using a sync.Pool for message buffers.

@whyrusleeping whyrusleeping added the status/in-progress In progress label Oct 28, 2014
@whyrusleeping
Copy link
Member Author

Travis build fails because sync.Pool only exists in 1.3+

@btc
Copy link
Contributor

btc commented Oct 28, 2014

Let's update the install instructions to Go 1.3+
https://github.com/jbenet/go-ipfs/blob/master/README.md#install

s.BufPool.New = func() interface{} {
return make([]byte, maxMsgLen)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This sync.Pool allocation should happen only on NewChan, otherwise, i can pass use NewChanWithPool(n, something) and if something is nil, it would not fail, and would be really, really hard to track down.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm... that is fair... What do you suggest?

@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

  1. The msgio changes should be merged into https://github.com/jbenet/go-msgio and then re-vendor this -- we can discuss them here though.
  2. I broke your signed pipe, and i think i also fixed it.

@@ -228,3 +228,21 @@ func (n *DAGService) Remove(nd *Node) error {
}
return n.Blocks.DeleteBlock(k)
}

func FetchGraph(ctx context.Context, root *Node, serv *DAGService) {
Copy link
Member

Choose a reason for hiding this comment

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

s/Fetch/Get/. Fetch is weird.

And, add a comment. Get a linter package for your editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose fetch because it matches the semantics of git fetch as this only grabs the data in this DAG and stores it locally, and doesnt update or return anything

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok. You had me at git fetch 😄

@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

@whyrusleeping ok, CRed

@@ -19,13 +21,35 @@ func NewChan(chanSize int) *Chan {
}
}

func NewChanWithPool(chanSize int, pool *sync.Pool) *Chan {
Copy link
Member

Choose a reason for hiding this comment

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

@whyrusleeping I merged jbenet/go-msgio#2 -- vendor this

@jbenet
Copy link
Member

jbenet commented Oct 31, 2014

@whyrusleeping very useful pr, can't wait to have it land.

  • this is one of those PRs that packs a ton in and got huge.
  • (:+1: to @maybebtc's diligence in splitting everything into independent PRs. maybe we should all strive for that).
  • I've reviewed things. A few still need addressing.
  • The signed pipe is not yet right. that will take a bit, so let's split it off into a separate PR and merge in the rest of these (much needed!) changes
  • (use rebase -i to reorder commits for easy splits)

@jbenet
Copy link
Member

jbenet commented Nov 2, 2014

This PR introduces (or maybe it was already there) use of sync.Pool, which appeared in Go 1.3. This kills Go 1.2 support. I'll remove it from travis now.

Go 1.2 is no longer supported (we use sync.Pool).
jbenet added a commit that referenced this pull request Nov 2, 2014
Performance optimizations and memory management
@jbenet jbenet merged commit 03ca7fd into master Nov 2, 2014
@jbenet jbenet removed the status/in-progress In progress label Nov 2, 2014
@jbenet
Copy link
Member

jbenet commented Nov 2, 2014

@whyrusleeping i merged this

@jbenet jbenet deleted the bench branch November 2, 2014 01:26
@whyrusleeping
Copy link
Member Author

<3

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.

3 participants