-
Notifications
You must be signed in to change notification settings - Fork 1
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
enhance #10 : avoid allocations on parseUUID
#12
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Flags with carried forward coverage won't be shown. Click here to find out more.
|
parseUUID
helper.go
Outdated
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") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") | |
} |
There was a problem hiding this comment.
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
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") | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or this
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
// 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++ | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
is this changes in some parts goes to google/uuid repo pr ? |
Let me see what I can do best there. I had the flexibility of writing this repo in my preferred style and structure. With |
Sadly, people aren't reviewing the proposal there. It'd be great if the PR gets some attention. |
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