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

proposal: crypto/sha256: satisfy io.StringWriter #34904

Closed
hummerd opened this issue Oct 14, 2019 · 4 comments
Closed

proposal: crypto/sha256: satisfy io.StringWriter #34904

hummerd opened this issue Oct 14, 2019 · 4 comments

Comments

@hummerd
Copy link

@hummerd hummerd commented Oct 14, 2019

Hi, everyone!
Let's implement io.StringWriter for sha256 digest.

// Yes this looks dangerous
// But there is no way for d.Write(p) to change p (now or ever) 
// since this will break backward compatability, also we can guarantee
// imutability of s with test
func (d *digest) WriteString(s string) (nn int, err error) {
	hString := (*reflect.StringHeader)(unsafe.Pointer(&s))
	hSlice := &reflect.SliceHeader{
		Data: hString.Data,
		Len:  hString.Len,
		Cap:  hString.Len,
	}
	p := *(*[]byte)(unsafe.Pointer(hSlice))
	return d.Write(p)
}

This will let us execute this benchmarks 17% faster and with 0 allocs/op (tested with go 13.1)

var testString string = "some very very long string that will cause allocation, when converted to []byte"

// regular string hashing
func BenchmarkHash(b *testing.B) {
	h := sha256.New()

	for i := 0; i < b.N; i++ {
		_, _ = h.Write([]byte(testString))
	}
}

// string hashing with io.StringWriter
func BenchmarkHashString(b *testing.B) {
	h := sha256.New()
	sw := h.(io.StringWriter)

	for i := 0; i < b.N; i++ {
		_, _ = sw.WriteString(testString)
	}
}
@gopherbot gopherbot added this to the Proposal milestone Oct 14, 2019
@gopherbot gopherbot added the Proposal label Oct 14, 2019
@agnivade agnivade changed the title proposal: Some tricky optimization in crypto sha256 proposal: crypto/sha256: satisfy io.StringWriter Oct 15, 2019
@josharian

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented Oct 15, 2019

Also proposed at #14757

@ainar-g

This comment has been minimized.

Copy link
Contributor

@ainar-g ainar-g commented Oct 17, 2019

This is wildly unsafe:

p := *(*[]byte)(unsafe.Pointer(&s))

See unsafe rule №1:

(1) Conversion of a *T1 to Pointer to *T2.

Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, (…)

In fact, this very same comment shows exactly how this conversion must be done:

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n
@hummerd

This comment has been minimized.

Copy link
Author

@hummerd hummerd commented Oct 17, 2019

This is wildly unsafe:

p := *(*[]byte)(unsafe.Pointer(&s))

See unsafe rule №1:

(1) Conversion of a *T1 to Pointer to *T2.

Provided that T2 is no larger than T1 and that the two share an
equivalent memory layout, (…)

In fact, this very same comment shows exactly how this conversion must be done:

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n

Changed WriteString implementation to more correct one with proper capacity initialization

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 2, 2019

Closing as duplicate of #14757. It would be useful to mention real world use cases for it in that issues.

@FiloSottile FiloSottile closed this Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.