-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
connect: reduce allocations when building SET command #1111
connect: reduce allocations when building SET command #1111
Conversation
connection.go
Outdated
params = append(params, param+"="+val) | ||
if cmdSet == nil { | ||
// Heuristic: 29 chars for each other key=value to reduce reallocations | ||
cmdSet = make([]byte, 0, 4+len(param)+1+len(val)+30*(len(mc.cfg.Params)-1)) |
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.
Wouldn't it make sense to use either a const-sized buffer on the heap for the common case or try to use the mc.buf
?
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.
A better memory saving would occur by building the SET NAMES
and SET
commands just once for all connections as the DSN is frozen, and store them in the connector
. But this is more profound refactoring.
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.
#1115 is the first step to move the connection-independent parts of handleParams
to a connector
constructor.
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 use strings.Builder.
https://golang.org/pkg/strings/#Builder
OK, |
@methane Code pushed using strings.Builder. |
9f37b88
to
acb481b
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.
It's an improvement and good to merge, but I think we should continue with the approach where we only build this string only once per DSN.
…1111) * connect: reduce allocations when building SET command * handleParams: use strings.Builder instead of direct []byte
…1111) * connect: reduce allocations when building SET command * handleParams: use strings.Builder instead of direct []byte
Description
As a follow-up to #1099 (see #1099 (comment)), here is an improvement of the
handleParams
function to reduce number of allocations when building theSET
command to set session variables.Checklist