Skip to content

connect: reduce allocations when building SET command#1111

Merged
julienschmidt merged 2 commits intogo-sql-driver:masterfrom
dolmen:connect-params-reduce-string-alloc
May 29, 2020
Merged

connect: reduce allocations when building SET command#1111
julienschmidt merged 2 commits intogo-sql-driver:masterfrom
dolmen:connect-params-reduce-string-alloc

Conversation

@dolmen
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

dolmen commented May 27, 2020

@methane Code pushed using strings.Builder.

@dolmen dolmen force-pushed the connect-params-reduce-string-alloc branch from 9f37b88 to acb481b Compare May 28, 2020 15:40
Copy link
Copy Markdown
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.

3 participants