-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Remove for loop in parseSize to enable inlining #580
Conversation
👍 |
lgtm |
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.
Except for a nitpick, lgtm.
server/util.go
Outdated
var i, l int | ||
var dec byte | ||
l = len(d) | ||
if l == 0 { |
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.
nitpick: Could you replace var declarations above with this:
l := len(d)
if l == 0 {
return -1
}
var (
i int
dec byte
)
(assuming that this does not affect performance)
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.
Thanks, updated and confirmed we also get a small performance boost:
benchmark old ns/op new ns/op delta
BenchmarkParseSize-4 8.81 5.73 -34.96%
Benchmark_____Pub0b_Payload-4 92.9 85.0 -8.50%
Benchmark_____Pub8b_Payload-4 96.1 85.4 -11.13%
Benchmark____Pub32b_Payload-4 106 99.3 -6.32%
Benchmark___Pub128B_Payload-4 131 122 -6.87%
Benchmark___Pub256B_Payload-4 152 146 -3.95%
Benchmark_____Pub1K_Payload-4 348 332 -4.60%
Benchmark_____Pub4K_Payload-4 1474 1428 -3.12%
Benchmark_____Pub8K_Payload-4 3330 3257 -2.19%
benchmark old MB/s new MB/s speedup
BenchmarkParseSize-4 113.53 174.60 1.54x
Benchmark_____Pub0b_Payload-4 118.45 129.37 1.09x
Benchmark_____Pub8b_Payload-4 197.74 222.43 1.12x
Benchmark____Pub32b_Payload-4 413.47 442.96 1.07x
Benchmark___Pub128B_Payload-4 1072.63 1154.81 1.08x
Benchmark___Pub256B_Payload-4 1765.96 1835.92 1.04x
Benchmark_____Pub1K_Payload-4 2979.37 3117.37 1.05x
Benchmark_____Pub4K_Payload-4 2788.11 2877.11 1.03x
Benchmark_____Pub8K_Payload-4 2464.19 2519.17 1.02x
Using a goto based loop makes it become a leaf function which can be inlined, making us get a slight performance increase in the fast path. See: golang/go#14768
d19586b
to
cd86c99
Compare
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.
LGTM.
Using a
goto
based loop makes it become a leaf function which can be inlined, making us get a slight performance increase as this is in the fast path:Local benchmarks:
Resolves #NNN
git pull --rebase origin master
)/cc @nats-io/core