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

enhance #10 : avoid allocations on parseUUID #12

Merged

Conversation

ashwingopalsamy
Copy link
Contributor

Changes

With this PR, we rewrite parseUUID to process input directly, avoiding unnecessary string allocations when handling UUIDs with -. The function now uses a pre-allocated byte slice and skips dashes inline to improve its efficiency, with few additional checks.

This should address the issue raised in #10. Let me know your thoughts! @vtolstov

@ashwingopalsamy ashwingopalsamy added the enhancement Improvements to existing feature or configuration label Dec 24, 2024
@ashwingopalsamy ashwingopalsamy self-assigned this Dec 24, 2024
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
unittests 90.67% <100.00%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
helper.go 100.00% <100.00%> (ø)

@ashwingopalsamy ashwingopalsamy changed the title enhance #10 : enhance #10 : avoid allocations on parseUUID Dec 24, 2024
helper.go Outdated
Comment on lines 33 to 43
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

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

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)

helper.go Outdated
Comment on lines 45 to 54
// 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

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.

uuidv8_test.go Outdated
"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.

@ashwingopalsamy ashwingopalsamy merged commit 4ccd19c into main Dec 26, 2024
2 checks passed
@vtolstov
Copy link

is this changes in some parts goes to google/uuid repo pr ?

@ashwingopalsamy
Copy link
Contributor Author

Let me see what I can do best there. I had the flexibility of writing this repo in my preferred style and structure.

With google/uuid, I had to follow what was already in-place..

@ashwingopalsamy
Copy link
Contributor Author

Sadly, people aren't reviewing the proposal there. It'd be great if the PR gets some attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing feature or configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants