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.

Sign up for GitHub

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

Jump to bottom

hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions #63719

Closed
apocelipes opened this issue Oct 25, 2023 · 4 comments
Closed

hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions #63719

apocelipes opened this issue Oct 25, 2023 · 4 comments
Labels
FrozenDueToAge help wanted
Milestone
Unplanned

Comments

@apocelipes
Copy link
Contributor

apocelipes commented Oct 25, 2023 •
edited
Loading

CL#66710 implements the BinaryMarshaler and BinaryUnmarshaler interfaces, which introduce handwritten helper functions like appendUint32/appendUint64/readUint32/readUint64.

For example, look at hash/crc64/crc64.go:

func appendUint64(b []byte, x uint64) []byte {
	a := [8]byte{
		byte(x >> 56),
		byte(x >> 48),
		byte(x >> 40),
		byte(x >> 32),
		byte(x >> 24),
		byte(x >> 16),
		byte(x >> 8),
		byte(x),
	}
	return append(b, a[:]...)
}

func readUint64(b []byte) uint64 {
	_ = b[7]
	return uint64(b[7]) | uint64(b[6])<<8 | uint64(b[5])<<16 | uint64(b[4])<<24 |
		uint64(b[3])<<32 | uint64(b[2])<<40 | uint64(b[1])<<48 | uint64(b[0])<<56
}

There are many of the same helper functions in other packages.

Since CL#386017, these functions become the public interface to the binary standard library.

Code using these helper functions can be easily rewritten, using crc64 as an example:

Before:

func (d *digest) MarshalBinary() ([]byte, error) {
	b := make([]byte, 0, marshaledSize)
	b = append(b, magic...)
	b = appendUint64(b, tableSum(d.tab))
	b = appendUint64(b, d.crc)
	return b, nil
}

func (d *digest) UnmarshalBinary(b []byte) error {
	if len(b) < len(magic) || string(b[:len(magic)]) != magic {
		return errors.New("hash/crc64: invalid hash state identifier")
	}
	if len(b) != marshaledSize {
		return errors.New("hash/crc64: invalid hash state size")
	}
	if tableSum(d.tab) != readUint64(b[4:]) {
		return errors.New("hash/crc64: tables do not match")
	}
	d.crc = readUint64(b[12:])
	return nil
}

After:

func (d *digest) MarshalBinary() ([]byte, error) {
	b := make([]byte, 0, marshaledSize)
	b = append(b, magic...)
	b = binary.BigEndian.AppendUint64(b, tableSum(d.tab))
	b = binary.BigEndian.AppendUint64(b, d.crc)
	return b, nil
}

func (d *digest) UnmarshalBinary(b []byte) error {
	if len(b) < len(magic) || string(b[:len(magic)]) != magic {
		return errors.New("hash/crc64: invalid hash state identifier")
	}
	if len(b) != marshaledSize {
		return errors.New("hash/crc64: invalid hash state size")
	}
	if tableSum(d.tab) != binary.BigEndian.Uint64(b[4:]) {
		return errors.New("hash/crc64: tables do not match")
	}
	d.crc = binary.BigEndian.Uint64(b[12:])
	return nil
}

There are these reasons below to support this refactoring:

  • Reducing duplicate code and preventing code bloat
  • Allows for a clearer indication of the data encoding method used (big-endian or little-endian)
  • A cleaner implementation of AppendUint in the binary package (no need for temporary arrays)
  • None of the modifications are public api's and will have no impact on language and standard library usage.

Packages that need to be refactored:

  • hash/adler32
  • hash/crc32
  • hash/crc64
  • hash/fnv
@gopherbot gopherbot added this to the Proposal milestone Oct 25, 2023
@randall77
Copy link
Contributor

Taking out of the proposal process because there's no new API here.
Just do it, details can be argued over in CLs.

@randall77 randall77 modified the milestones: Proposal, Unplanned Oct 25, 2023
@apocelipes apocelipes changed the title proposal: crypto, hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions crypto, hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions Oct 25, 2023
@apocelipes
Copy link
Contributor Author

Taking out of the proposal process because there's no new API here. Just do it, details can be argued over in CLs.

Thanks. I will send a PR later.

@apocelipes
Copy link
Contributor Author

apocelipes commented Oct 25, 2023 •
edited
Loading

There are some import cycles between hash and encoding/binary. So only we can do is to keep the helper functions as same as that in the binary package and let other code keep them it as.

@apocelipes apocelipes changed the title crypto, hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions hash: should use binary.BigEndian's AppendUint{32|64} and Uint{32|64} instead of handwritten helper functions Oct 26, 2023
apocelipes added a commit to apocelipes/go that referenced this issue Oct 26, 2023
@apocelipes
hash: simplify binary operations
6af1d54 
Can not use encoding/binary in hash packages because of import cycles.

So just keep the appendUint and readUint helper functions same as that
in the encoding/binary standard package.

Updates golang#63719
@apocelipes apocelipes mentioned this issue Oct 26, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/537796 mentions this issue: hash: simplify binary operations

apocelipes added a commit to apocelipes/go that referenced this issue Oct 26, 2023
@apocelipes
hash: simplify binary operations
f334fee 
Can not use encoding/binary in hash packages because of import cycles.

So just keep the appendUint and readUint helper functions same as that
in the encoding/binary standard package.

Updates golang#63719
gopherbot pushed a commit that referenced this issue Nov 19, 2023
@apocelipes @randall77
hash: simplify binary operations
1c15291 
We can not use encoding/binary in hash packages because of import cycles.

So just keep the appendUint and readUint helper functions same as that
in the encoding/binary standard package.

There is no notable performance impacts.

Updates #63719

Change-Id: If47a7faaf9d422d772f32bbe1fa2f2c8a16485f4
GitHub-Last-Rev: f334fee
GitHub-Pull-Request: #63746
Reviewed-on: https://go-review.googlesource.com/c/go/+/537796
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@randall77 @gopherbot @apocelipes