make putNode correct

[zhiqiangxu]

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 

[jarifibrahim]

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.

[zhiqiangxu]

node address = address of first element + offset

In order to make sure node address is aligned, should consider address of first element, which is not necessarily aligned.

It's wrong to only consider offset

[zhiqiangxu]

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 []byte is 8 byte aligned, though the first element of []uint64 is always 8 byte aligned.

[stale[bot]]

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.

[jarifibrahim]

@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.

[stale[bot]]

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.

[jarifibrahim]

@zhiqiangxu I did a small benchmark to compare the performance. This is the BenchmarkWrite function in the skl package.

name            old time/op    new time/op    delta
Write-8          522ns ± 8%     563ns ± 3%  +7.87%  (p=0.001 n=10+8)

The new changes seem to be slower. Do you know why it could be slower?

[zhiqiangxu]

Maybe because of the extra bounding check and more bytes involved, but pity that I don't know how to optimize it:(

[damz]

The rationale looks correct to me: Golang doesn't guarantee that the underlying memory block of a []byte is aligned. It only guarantees that the underlying memory block of an array follows the alignment guarantees of each its elements:

From https://golang.org/ref/spec#Size_and_alignment_guarantees:

For a variable x of array type: unsafe.Alignof(x) is the same as the alignment of a variable of the array's element type.

So a []uint64 will be aligned to 64 bit on platforms that requires that. But a []byte will not.

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.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}