-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
make putNode correct #1272
base: master
Are you sure you want to change the base?
make putNode correct #1272
Conversation
Hey @zhiqiangxu, can you add some more information to the PR description? I'm trying to understand why do we need this change and what are the implications of it. |
node address = address of first element + offset In order to make sure It's wrong to only consider |
The spec only guarantees : the first (64-bit) word in a variable or in an allocated struct, array, or slice can be relied upon to be 64-bit aligned. So it's not guaranteed that the first element of a |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@zhiqiangxu The review for this PR is pending because I don't understand alignment issues properly. I'll read some stuff about it and then review this PR. Apologies for the delay. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@zhiqiangxu I did a small benchmark to compare the performance.
The new changes seem to be slower. Do you know why it could be slower? |
Maybe because of the extra bounding check and more bytes involved, but pity that I don't know how to optimize it:( |
The rationale looks correct to me: Golang doesn't guarantee that the underlying memory block of a From https://golang.org/ref/spec#Size_and_alignment_guarantees:
So a Based on this, I can suggest the following: func makeAligned(n int64) []byte {
buf := make([]uint64, n>>3)
head := (*reflect.SliceHeader)(unsafe.Pointer(&buf))
head.Len = int(n)
head.Cap = int(n)
return *(*[]byte)(unsafe.Pointer(head))
} It doesn't seem to have any measurable performance impact. |
|
This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open. |
Keep open |
s.buf
bytes may be not aligned, so the correct implementation should take that into consideration.Update
Switched to use
makeAlignedBuf
instead.This change is