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

Copy buffer bytes #864

Merged
merged 1 commit into from Oct 25, 2022
Merged

Copy buffer bytes #864

merged 1 commit into from Oct 25, 2022

Conversation

bill-rich
Copy link
Collaborator

buffer.Bytes() gives a pointer to the slice that is being reused. Need to copy the slice to avoid weirdness later.

@bill-rich bill-rich requested a review from a team as a code owner October 25, 2022 01:16
Copy link
Contributor

@dustin-decker dustin-decker left a comment

Choose a reason for hiding this comment

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

Preallocating the slice and using copy would be better here

https://gist.github.com/xogeny/b819af6a0cf8ba1caaef

@trufflesteeeve
Copy link
Collaborator

Based on runs of the metrics, it seems like whatever you do, either copy or append, is good so long as you preallocate the destination slice.

@bill-rich
Copy link
Collaborator Author

I thought pre-allocating should be the fastest too. For some reason in this case append seems to be slightly faster.

With append on my test repo:
32.593306s - 32.818896 seconds

With make and copy:
32.906427 - 33.12590 seconds

Not a huge difference, but I've been getting those results consistently. I'll merge as is for now to get things fixed and run some tests against a really large repo to see how the two compare.

@bill-rich bill-rich merged commit d7d614c into main Oct 25, 2022
@bill-rich bill-rich deleted the git_scan_fix branch October 25, 2022 16:09
@dustin-decker
Copy link
Contributor

There are probably some external variables there, the clean benchmarks show that pre-allocation with either append or copy is faster.

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

Successfully merging this pull request may close these issues.

None yet

3 participants