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

connect: reduce allocations when building SET command #1111

Merged

Conversation

dolmen
Copy link
Contributor

@dolmen dolmen commented May 22, 2020

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 the SET command to set session variables.

Checklist

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))
Copy link
Member

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?

Copy link
Contributor Author

@dolmen dolmen May 27, 2020

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.

Copy link
Contributor Author

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.

Copy link
Member

@methane methane left a 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

@dolmen
Copy link
Contributor Author

dolmen commented May 27, 2020

Please use strings.Builder.
https://golang.org/pkg/strings/#Builder

OK, less efficient, but I'm changing the code.

@dolmen
Copy link
Contributor Author

dolmen commented May 27, 2020

@methane Code pushed using strings.Builder.

connection.go Outdated Show resolved Hide resolved
@dolmen dolmen force-pushed the connect-params-reduce-string-alloc branch from 9f37b88 to acb481b Compare May 28, 2020 15:40
Copy link
Member

@julienschmidt julienschmidt left a 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.

@julienschmidt julienschmidt merged commit 26060e1 into go-sql-driver:master May 29, 2020
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
…1111)

* connect: reduce allocations when building SET command

* handleParams: use strings.Builder instead of direct []byte
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
…1111)

* connect: reduce allocations when building SET command

* handleParams: use strings.Builder instead of direct []byte
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants