Skip to content

Conversation

@RafaelCenzano
Copy link
Contributor

GODRIVER-3707

Summary

  • Moved and improved append functions for 32 and 64 signed and unsigned integers into bytes into binaryutils package
  • Moved read function for 32 and 64 signed and unsigned integers into binaryutils package
  • Moved and consolidated read c string function into binaryutils package

Background & Motivation

There was duplicate code across the bsoncore and wiremessage packages. This PR consolidates it and made some minor improvements including better/safer appending thanks to work from @prestonvasquez and instead of stating read c string functions must be synced, use the same core logic and wrap a cast on the response from the bytes version for the string version.

RafaelCenzano and others added 15 commits December 19, 2025 18:00
Create new binaryutil package
Create new Append32 function based off PutUint32 from go binary standard
library
Replace use of appendu32 function from bson x package

Co-authored-by: Preston Vasquez <[email protected]>
Replace use of appendi32 function from bson x package and x/mongo/driver/wiremessage/wiremessage
Create new Append64 function based off PutUint64 from go binary standard
library

Co-authored-by: Preston Vasquez <[email protected]>
Replace use of appendi64 function from these packages
x/mongo/driver/wiremessage
x/bsonx/bsoncore
Replace use of appendu64 function from these packages
x/mongo/driver/wiremessage
x/bsonx/bsoncore
Create function that can read uint32 and int32 from byte slice
Go can't infer types from return types which means types need to be
explicit for binary read functions
Instead I created read functions for the unsigned and signed types that
call the generic with a type
Replace readu32 with new ReadU32 function in internal binary package
Replace readi32 in bson and wiremessage package with new ReadI32
function in the internal binary package
Create new generic function to read uint64 and int64 from binary slice
Create new wrapper functions for unsigned and signed int calls
Replace use of readu64 in bson package with new ReadU64 function in the
internal binary utils package
Replace use of readi64 in bson and wiremessage package with
new ReadI64 function in the internal binary utils package
Created a ReadCStringBytes function that reads bytes from a byte slice
Created a ReadCString function that wraps bytes returned from
ReadCStringBytes as a string
Replace readcstringbytes function from bson package with
ReadCStringBytes function from binary utils package
Replace readcstring function from bson and wiremessage package with
ReadCString function from binary utils package
@RafaelCenzano RafaelCenzano added enhancement review-priority-normal Medium Priority PR for Review: within 1 business day go Pull requests that update Go code labels Dec 22, 2025
@mongodb-drivers-pr-bot
Copy link
Contributor

API Change Report

No changes found!

@mongodb-drivers-pr-bot
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Dec 22, 2025

🧪 Performance Results

Commit SHA: f238fb7

The following benchmark tests for version 6961a7b61501750007e273cb had statistically significant changes (i.e., |z-score| > 1.96):

Benchmark Measurement % Change Patch Value Stable Region H-Score Z-Score
BenchmarkBSONDeepDocumentEncoding ops_per_second_min 30.7722 4482.0739 Avg: 3427.3892
Med: 3425.2323
Stdev: 505.9931
0.7438 2.0844
BenchmarkSmallDocInsertOne allocated_bytes_per_op 0.3086 5688.0000 Avg: 5670.5000
Med: 5670.5000
Stdev: 2.1213
0.9571 8.2496

For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch.

@RafaelCenzano RafaelCenzano marked this pull request as ready for review December 22, 2025 23:51
Copilot AI review requested due to automatic review settings December 22, 2025 23:51
@RafaelCenzano RafaelCenzano requested a review from a team as a code owner December 22, 2025 23:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR consolidates duplicate binary utility functions from the bsoncore and wiremessage packages into a new shared binaryutil package, reducing code duplication and improving maintainability.

Key Changes:

  • Created a new internal/binaryutil package with generic implementations of 32-bit and 64-bit integer append/read functions
  • Consolidated C-string reading functions to use a single implementation
  • Updated all callers in bsoncore and wiremessage packages to use the new utility functions

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/binaryutil/binaryutil.go New utility package containing generic Append32/64, ReadI32/I64, ReadU32/U64, and ReadCString functions
x/mongo/driver/wiremessage/wiremessage.go Updated to use binaryutil functions; removed duplicate implementations of appendi32, appendi64, readi32, readi64, and readcstring
x/mongo/driver/wiremessage/wiremessage_test.go Updated test calls to use binaryutil.Append32/64 and ReadI32/64
x/bsonx/bsoncore/bsoncore.go Updated to use binaryutil functions; removed duplicate implementations of append/read functions for signed and unsigned 32/64-bit integers and C-strings
x/bsonx/bsoncore/document.go Updated ReadI32 call to use binaryutil.ReadI32

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// AppendGetMoreZero appends the zero field to dst.
func AppendGetMoreZero(dst []byte) []byte {
return appendi32(dst, 0)
return binaryutil.Append32(dst, int32(0))
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The explicit cast to int32(0) is redundant since the literal 0 can be inferred as int32 by the generic type parameter T in Append32. Consider simplifying to just 0.

Suggested change
return binaryutil.Append32(dst, int32(0))
return binaryutil.Append32(dst, 0)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this cast leads to this IDE error: int does not satisfy ~uint32 | ~int32 (int missing in ~uint32 | ~int32)

Copy link
Member

Choose a reason for hiding this comment

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

@RafaelCenzano This is a straight up compile error 😄. Might be worth downvoting and letting copilot know 🤷

// AppendKillCursorsZero appends the zero field to dst.
func AppendKillCursorsZero(dst []byte) []byte {
return appendi32(dst, 0)
return binaryutil.Append32(dst, int32(0))
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The explicit cast to int32(0) is redundant since the literal 0 can be inferred as int32 by the generic type parameter T in Append32. Consider simplifying to just 0.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

return dst
}

func read32[T ~uint32 | ~int32](src []byte) (T, []byte, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] The current generic version relies on binary.LittleEndian.Uint32 plus a uint32 -> int32 conversion, which works only because we're implicitly depending on overflow/wraparound to preserve the bit pattern. That's easy to misread. readi32 makes the intent explicit ("read a little‑endian int32"), doesn't hide behavior behind a generic cast, and is simpler to reason about and maintain.

As you've noted, both solutions do the same thing. So this is just a semantic recommendation that's geared toward intention and security (gosec 115).

Replace unsafe generic read bytes function with explict bit shifts with
the stated type.
This is meant to make things clearer for end consumers of the function
and maintainers follow gosec 115
(https://github.com/securego/gosec?tab=readme-ov-file#available-rules)
Addresses mongodb#2274 (comment)

Co-authored-by: Preston Vasquez <[email protected]>
"bytes"
)

func Append32[T ~uint32 | ~int32](dst []byte, v T) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] Each function should have a description. In the description we should note why we do byte shifting directly, instead of using the binary package. We should also note that the order is little-endian:

Append32 appends a uint32 or int32 value to dst in little-endian byte order. Byte shifting is done directly to prevent overflow security errors, in compliance with gosec 115.

See here: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=84

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added

// compliance with gosec G115.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=84
func Append32[T ~uint32 | ~int32](dst []byte, v T) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] There is a significant performance cost of this function compared to the standard library (0.5335 ns/op v 0.1760 ns/op):

❯ go test -bench=BenchmarkAppend32
goos: darwin
goarch: arm64
pkg: go.mongodb.org/mongo-driver/v2/internal/binaryutil
cpu: Apple M1 Pro
BenchmarkAppend32-10            1000000000               0.5335 ns/op   7498.15 MB/s
PASS
ok      go.mongodb.org/mongo-driver/v2/internal/binaryutil      0.907s
❯ go test -bench=BenchmarkStdlibAppendUint32
goos: darwin
goarch: arm64
pkg: go.mongodb.org/mongo-driver/v2/internal/binaryutil
cpu: Apple M1 Pro
BenchmarkStdlibAppendUint32-10          1000000000               0.1760 ns/op   22726.89 MB/s

Suggest copying the standard library directly:

func Append32[T ~uint32 | ~int32](dst []byte, v T) []byte {
	return append(dst,
		byte(v),
		byte(v>>8),
		byte(v>>16),
		byte(v>>24),
	)
}

This problem is also present in append64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if this is an incorrect way to do this I got the commands from AI, but when I run the benchmark my results are very close between the new implementation and standard library. Here is my output for averages over 100 runs from the append 32 functions:

go test ./internal/binaryutil/... -bench=BenchmarkAppend32 -benchmem -count=100 > bench1.out
go test ./internal/binaryutil/... -bench=BenchmarkStdlibAppendUint32 -benchmem -count=100 > bench2.out
awk '/BenchmarkAppend32-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench1.out
avg ns/op = 0.023329
awk '/BenchmarkStdlibAppendUint32-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench2.out
avg ns/op = 0.0234163

For append 32 those numbers are nearly the same over 100 runs.

For append 64 I do see a very drastic difference:

go test ./internal/binaryutil/... -bench=BenchmarkAppend64 -benchmem -count=100 > bench3.out
go test ./internal/binaryutil/... -bench=BenchmarkStdlibAppendUint64 -benchmem -count=100 > bench4.out
awk '/BenchmarkAppend64-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench3.out
avg ns/op = 0.128843
awk '/BenchmarkStdlibAppendUint64-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench4.out
avg ns/op = 0.0231089

To keep things uniform I will apply your suggestion to both functions, thanks for catching that

Copy link
Member

@prestonvasquez prestonvasquez Jan 5, 2026

Choose a reason for hiding this comment

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

I'm seeing something similar using benchstat. Regardless, it would be ideal to keep the code as close to the source as possible.

// avoid unsafe casting between unsigned and signed integers.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=79
func ReadU32(src []byte) (uint32, []byte, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] This function should wrap the standard library function since doing so doesn't require a type conversion:

func ReadU32(src []byte) (uint32, []byte, bool) {
	if len(src) < 4 {
		return 0, src, false
	}

	return binary.LittleEndian.Uint32(src), src[4:], true
}

// avoid unsafe casting between unsigned and signed integers.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=101
func ReadU64(src []byte) (uint64, []byte, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] This function should wrap the standard library function since doing so doesn't require a type conversion.

Major difference in performance on append 64 function versus
standard library code
dst = binaryutil.Append32(dst, length)
dst = binaryutil.Append32(dst, reqid)
dst = binaryutil.Append32(dst, respto)
dst = binaryutil.Append32(dst, int32(opcode))
Copy link
Member

Choose a reason for hiding this comment

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

[nit] We don't need to do a type conversion here. ~ as a constraint matches all types that have the same underlying type as the specified type, including user-defined types based on a primitive.

Same with the other opcode cases.

// AppendQueryFlags appends the flags for an OP_QUERY wire message.
func AppendQueryFlags(dst []byte, flags QueryFlag) []byte {
return appendi32(dst, int32(flags))
return binaryutil.Append32(dst, int32(flags))
Copy link
Member

Choose a reason for hiding this comment

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

[nit] We don't need to do a type conversion here. ~ as a constraint matches all types that have the same underlying type as the specified type, including user-defined types based on a primitive.

Same with the other flags cases.

// AppendGetMoreZero appends the zero field to dst.
func AppendGetMoreZero(dst []byte) []byte {
return appendi32(dst, 0)
return binaryutil.Append32(dst, int32(0))
Copy link
Member

Choose a reason for hiding this comment

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

@RafaelCenzano This is a straight up compile error 😄. Might be worth downvoting and letting copilot know 🤷

Comment on lines 47 to 52
// ReadU32 reads a uint32 from src in little-endian byte order.
// Byte shifting is done directly to prevent overflow security errors, in
// compliance with gosec G115. ReadU32 and ReadI32 are separate functions to
// avoid unsafe casting between unsigned and signed integers.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=79
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] We can simplify this comment since we aren't doing manual byte shifting.

Comment on lines 82 to 87
// ReadU64 reads a uint64 from src in little-endian byte order.
// Byte shifting is done directly to prevent overflow security errors, in
// compliance with gosec G115. ReadU64 and ReadI64 are separate functions to
// avoid unsafe casting between unsigned and signed integers.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=101
Copy link
Member

Choose a reason for hiding this comment

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

[blocking] We can update this comment since we aren't doing manual byte shifting.

// avoid unsafe casting between unsigned and signed integers.
//
// See: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/encoding/binary/binary.go;l=79
func ReadI32(src []byte) (int32, []byte, bool) {
Copy link
Member

@prestonvasquez prestonvasquez Jan 7, 2026

Choose a reason for hiding this comment

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

@RafaelCenzano Like we discussed in office hours, I can't tell why this isn't benchmarking the same as ReadU32 (more specifically, binary.LittleEndian.Uint32(src)) . But it appears to be the same locally: https://github.com/prestonvasquez/go-playground/blob/main/benchmark/binary_inline_test.go

Just noting here since it sounds like we should accept this solution but are seeing weirdness from the benchmarks.

CC: @matthewdale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gave ai a stab at the question, it's answer stated it may be related to the inlining. This may be wrong but it stated because the code in ReadU is inline already things are easy to compile and run while ReadI it inlines the standard library call, and then it may have issues properly optimizing the checks/hints like _ = src[3] // bounds check hint to compiler. I'm not familiar with the innerworkings of the benchmarking and testing framework in go. But could this be the reason for the difference?

Also as a note ReadU64 and ReadI64 have the same difference in benchmark

Copy link
Member

Choose a reason for hiding this comment

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

@RafaelCenzano The Go compiler would inline binary.LittleEndian.Uint32, which is evident in the results from the go-playground example.

Also as a note ReadU64 and ReadI64 have the same difference in benchmark

Could you elaborate on what you mean by this?

Copy link
Contributor Author

@RafaelCenzano RafaelCenzano Jan 8, 2026

Choose a reason for hiding this comment

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

When I ran the benchmark on those two functions I saw the same issue with performance differences.

go test ./internal/binaryutil/... -bench=BenchmarkReadI64 -benchmem -count=100 > bench1.out
go test ./internal/binaryutil/... -bench=BenchmarkReadU64 -benchmem -count=100 > bench2.out
awk '/BenchmarkReadI64-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench1.out
avg ns/op = 0.0235705
awk '/BenchmarkReadU64-/{sum+=$3; n++} END {print "avg ns/op =", sum/n}' bench2.out
avg ns/op = 0.0415546

We discussed in our meeting today. I'm going to spend a few hours looking into this before we merge to try to find a root cause if possible

Copy link
Member

@prestonvasquez prestonvasquez Jan 8, 2026

Choose a reason for hiding this comment

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

@RafaelCenzano Somehow the problem is this block:

if len(src) < 4 {
    return 0, src, false
}

The assembly ends up being [effectively] the same for either case, though. This is bizarre. I think we should just do type casting, despite the gosec issues. We should not introduce a regression.

  func ReadI32(src []byte) (int32, []byte, bool) {
      if len(src) < 4 {
          return 0, src, false
      }
      //nolint:gosec // G115: Safe bit-pattern reinterpretation
      return int32(binary.LittleEndian.Uint32(src)), src[4:], true
  }

Comment on lines 315 to 323
if len(got) != len(want) {
t.Errorf("Append32(%#x): len = %d, want %d", v, len(got), len(want))
return
}
for i := range got {
if got[i] != want[i] {
t.Errorf("Append32(%#x): byte %d = %#x, want %#x", v, i, got[i], want[i])
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Optional] We can use bytes.Equal() to simplify the bytes comparing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good callout, I will add it will simplify the code in the test file

Comment on lines 574 to 576
func readi32unsafe(src []byte) int32 {
return int32(binary.LittleEndian.Uint32(src))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need to keep this implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a great question originally there was not an exported function from the new binaryutils package but there is now.

ReadI32 has some more checks and things it seems like it can be used in place of this readi32unsafe. Is my understanding correct that it can be replaced with ReadI32 from the new binaryutils package?

context, this is one of the places where readi32unsafe is used right now: https://github.com/RafaelCenzano/mongo-go-driver/blob/refactor/godriver-3707-deduplicate-binary-funcs/x/mongo/driver/wiremessage/wiremessage.go#L237-L248

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think readi32unsafe can be replaced by ReadI32.

Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

Lets replace ReadI32 and ReadI64 w/ calls to the binary package analogues. It's unfortunate, but we can't introduce regressions here. Please leave a comment on both functions as to our findings.

Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@RafaelCenzano RafaelCenzano merged commit 48d8b49 into mongodb:master Jan 13, 2026
32 of 34 checks passed
@RafaelCenzano RafaelCenzano deleted the refactor/godriver-3707-deduplicate-binary-funcs branch January 13, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement go Pull requests that update Go code review-priority-normal Medium Priority PR for Review: within 1 business day

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants