Make PeerAddress/init faster
#37
Conversation
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Nice change! Left a few nits but we also need to be a little more careful parsing the port.
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
| guard elementValue >= 0, elementValue <= 9 else { | ||
| // non-digit character | ||
| return nil | ||
| } |
There was a problem hiding this comment.
At the moment if utf8Element is e.g. 47 then we'll underflow and crash when creating elementValue.
We should validate the UInt8 value and then do the conversion. It also means that the subtraction in the conversion can be unchecked.
There was a problem hiding this comment.
I thought maybe using a range instead was clearer so I've done, but I'm actually unsure if the range allocates or if it's all nicely optimised by the compiler.
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
One nit, but otherwise looks great!
| value &+= Int(utf8Char) | ||
| } | ||
|
|
||
| guard value <= 65535 else { |
There was a problem hiding this comment.
nit: let the compiler do the maths
| guard value <= 65535 else { | |
| guard value <= Int(UInt16.max) else { |
Sources/GRPCOTelTracingInterceptors/Tracing/SpanAttributes+GRPCTracingKeys.swift
Outdated
Show resolved
Hide resolved
| // non-digit character | ||
| return nil | ||
| } | ||
| value &+= Int(utf8Char - UInt8(ascii: "0")) |
There was a problem hiding this comment.
we can do unchecked here because we checked the range of values above
| value &+= Int(utf8Char - UInt8(ascii: "0")) | |
| value &+= Int(utf8Char &- UInt8(ascii: "0")) |
2 participants
PeerAddress/initcurrently usesStringandsplits them, which is not very optimal. This PR uses UTF8View instead to avoid allocations/make things faster