Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

feat: improve clone performance and reduce memory usage. #1170

Closed
wants to merge 8 commits into from

Conversation

orisano
Copy link
Contributor

@orisano orisano commented Jun 28, 2019

I tried to clone github.com/knqyf263/vuln-list, but PlainClone took a too long time.
I improve clone performance and reduce memory usage!

benchstat results

name          old time/op    new time/op    delta
PlainClone-4      605s ± 4%      173s ± 5%  -71.42%  (p=0.008 n=5+5)

name          old alloc/op   new alloc/op   delta
PlainClone-4    64.9GB ± 0%    29.8GB ± 0%  -54.12%  (p=0.008 n=5+5)

name          old allocs/op  new allocs/op  delta
PlainClone-4      117M ± 0%      114M ± 0%   -2.50%  (p=0.008 n=5+5)

thanks!

@orisano orisano force-pushed the feat-improve-clone-performance branch from 64f2fb7 to 76abe01 Compare June 28, 2019 19:08
@mcuadros mcuadros requested a review from jfontan June 29, 2019 10:46
@@ -20,8 +20,14 @@ func (s *IndexStorage) SetIndex(idx *index.Index) (err error) {
}

defer ioutil.CheckClose(f, &err)
bw := bufio.NewWriter(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you found substantial difference using a buffered writer to save the index?

Copy link
Contributor Author

@orisano orisano Jul 1, 2019

Choose a reason for hiding this comment

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

local benchmark result. reverted bufio.Writer.

name          old time/op    new time/op    delta
PlainClone-4      173s ± 5%      251s ± 9%  +44.84%  (p=0.008 n=5+5)

name          old alloc/op   new alloc/op   delta
PlainClone-4    29.8GB ± 0%    29.4GB ± 0%   -1.12%  (p=0.008 n=5+5)

name          old allocs/op  new allocs/op  delta
PlainClone-4      114M ± 0%      114M ± 0%   -0.04%  (p=0.008 n=5+5)

Copy link
Contributor

@jfontan jfontan 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 doing more tests with our repository downloader and these changes makes it a little bit slower and consumes more memory. I'll continue digging why this could be.

@jfontan
Copy link
Contributor

jfontan commented Jul 1, 2019

It seems that the biggest improvement are the changes in worktree.go and storage/filesystem/index.go. Can yo split this PR in two? One with changes for those files and another with changes for packfile code.

I've been reviewing the changes for packfile and make sense but in some cases they make the code slower. I'm pretty sure this can be improved to get good gains in both memory and speed when it's generating the index.

@mcuadros what do you think?

@orisano
Copy link
Contributor Author

orisano commented Jul 14, 2019

Ok, I split PR.

@orisano
Copy link
Contributor Author

orisano commented Jul 14, 2019

Shall I close this PR?

@jfontan
Copy link
Contributor

jfontan commented Jul 18, 2019

@orisano I would love to take a look to the changes for packfile you did. Even if they are not increasing speed now they look very promising. Can you open a new PR with only the changes from the files in plumbing/format/packfile/*. We can work from there to improve them.

Thanks!

@orisano
Copy link
Contributor Author

orisano commented Jul 18, 2019

ok

@jfontan
Copy link
Contributor

jfontan commented Jul 31, 2019

The changes from this PR are now in #1180

@jfontan jfontan closed this Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants