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

enhance #10 : avoid allocations on parseUUID #12

Merged
merged 5 commits into from
Dec 26, 2024
Merged

enhance #10 : avoid allocations on parseUUID #12

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
446522c
Update helper.go
ashwingopalsamy Dec 24, 2024
4023410
tests: added unit tests for updated `parseUUID`
ashwingopalsamy Dec 24, 2024
0f275bc
fix: merge with main
ashwingopalsamy Dec 26, 2024
7b52e05
tests: add more testcases for invalid uuid inputs
ashwingopalsamy Dec 26, 2024
4c42601
enhance: `parseUUID()` logic handling
ashwingopalsamy Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/hex"
"errors"
"fmt"
"strings"
)

// Helper function to encode timestamp into the UUID byte array.
Expand All @@ -31,12 +30,30 @@ func decodeTimestamp(uuidBytes []byte) uint64 {

// Helper function to parse and sanitize a UUID string.
func parseUUID(uuid string) ([]byte, error) {
uuid = strings.ReplaceAll(uuid, "-", "")
if len(uuid) != 32 {
if len(uuid) == 32 {
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
} else if len(uuid) == 36 {
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
} else {
return nil, errors.New("invalid UUID length")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(uuid) == 32 {
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
} else if len(uuid) == 36 {
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
} else {
return nil, errors.New("invalid UUID length")
}
switch len(uuid) {
case 32:
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
case 36:
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
default:
return nil, errors.New("invalid UUID length")
}

Copy link
Contributor

@ccoVeille ccoVeille Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, if you consider this

#12 (comment)

Suggested change
if len(uuid) == 32 {
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
} else if len(uuid) == 36 {
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
} else {
return nil, errors.New("invalid UUID length")
}
switch len(uuid) {
case 32:
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
case 36:
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
return hex.DecodeString(strings.ReplaceAll(uuid, "-", ""))
default:
return nil, errors.New("invalid UUID length")
}

Copy link
Contributor

@ccoVeille ccoVeille Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Or this

Suggested change
if len(uuid) == 32 {
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)
} else if len(uuid) == 36 {
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
} else {
return nil, errors.New("invalid UUID length")
}
if len(uuid) == 36 {
// Validate dash positions
if uuid[8] != '-' || uuid[13] != '-' || uuid[18] != '-' || uuid[23] != '-' {
return nil, errors.New("invalid UUID format")
}
uid = strings.ReplaceAll(uid, "-", "")
}
if len(uuid) != 32 {
return nil, errors.New("invalid UUID length")
}
// Fast path for UUIDs without dashes
return hex.DecodeString(uuid)


return hex.DecodeString(uuid)
// Remove dashes while copying characters
result := make([]byte, 32)
j := 0
for i := 0; i < len(uuid); i++ {
if uuid[i] == '-' {
continue
}
result[j] = uuid[i]
j++
}
Copy link
Contributor

@ccoVeille ccoVeille Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not a simple strings.ReplaceAll?

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 point! Primary reason was to address #10 . When I attempted 'Benchmarking', the current implementation was faster and avoids memory allocations.

Here are the benchmark results comparing the two:

Implementation Time per Operation (ns/op) Speed
strings.ReplaceAll() 146.2 ns/op Baseline
Current Approach 68.17 ns/op 2.22x faster

How?

This approach processes the UUID in one pass, directly copying characters into a pre-allocated byte slice. This avoids creating a new string, which is what strings.ReplaceAll does and keeps things lean.


return hex.DecodeString(string(result))
}

// Helper function to check if a UUID is all zeros.
Expand Down
53 changes: 53 additions & 0 deletions uuidv8_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,3 +561,56 @@ func TestNewWithParams_MaxValues(t *testing.T) {
t.Errorf("Generated UUID with max values is invalid: %s", uuid)
}
}

func TestFromString_InvalidInputs(t *testing.T) {
tests := []string{
"123", // Too short
"123e4567e89b12d3a4564266141740000000", // Too long
"123e4567e89b12d3a45642661417400g", // Invalid character
"123e-4567-e89b-12d3-a456-426614174000", // Misplaced dashes
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add:

  • a valid uid with the dashes at the right place
  • an invalid uid with the dash at the right place plus one randomly placed,or simply 36 dashes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing! Added in the latest commit.

}

for _, input := range tests {
t.Run("Invalid UUID "+input, func(t *testing.T) {
_, err := uuidv8.FromString(input)
if err == nil {
t.Errorf("Expected error, got nil for input: %s", input)
}
})
}
}

func TestFromStringOrNil_InvalidInputs(t *testing.T) {
tests := []string{
"123", // Too short
"123e4567e89b12d3a4564266141740000000", // Too long
"123e4567e89b12d3a45642661417400g", // Invalid character
"", // Empty string
}

for _, input := range tests {
t.Run("Invalid UUID "+input, func(t *testing.T) {
result := uuidv8.FromStringOrNil(input)
if result != nil {
t.Errorf("Expected nil for input: %s, got %v", input, result)
}
})
}
}

func TestIsValidUUIDv8_InvalidUUIDs(t *testing.T) {
invalidUUIDs := []string{
"123", // Too short
"123e4567e89b12d3a4564266141740000000", // Too long
"123e4567e89b12d3a45642661417400g", // Invalid character
"", // Empty string
}

for _, uuid := range invalidUUIDs {
t.Run("Invalid UUID "+uuid, func(t *testing.T) {
if uuidv8.IsValidUUIDv8(uuid) {
t.Errorf("Expected UUID %s to be invalid", uuid)
}
})
}
}
Loading