sqlbase: avoid allocations due to ErrNameString, speed up IMPORT by 10%#35317
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Mar 2, 2019
Merged
Conversation
Member
|
This change is |
Contributor
Author
|
Spotted by @danhhz 😃 |
danhhz
approved these changes
Mar 1, 2019
Contributor
danhhz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @danhhz and @nvanbenschoten)
pkg/sql/sem/tree/name_part.go, line 63 at r1 (raw file):
// identifier suitable for printing in error messages. func ErrNameString(s string) string { return ErrString(((*Name)(&s)))
I think you could replace the body of this with something like this, no?
var buf bytes.Buffer
lex.EncodeRestrictedSQLIdent(&buf, s, FmtBareIdentifiers.EncodeFlags())
return buf.String()
That would avoid the allocation in the error path, but more importantly, it would mean you could get rid of ErrNameStringP and make this all easier to use. It's pretty subtle that you have to know which of these two to call based on whether the string is already escaping or not.
Contributor
|
That still creates you an allocation (that of |
Contributor
|
Ah. I was thinking it would allocate that buf either way, but I hadn't dug down far enough to see fmtCtxPool. Nevermind what I said. |
Informs cockroachdb#34809. Before this change, `ErrNameString` only accepted a pointer to a string. This forced the owner of the string to escape. In the following two cases, this resulted in an entire `ColumnDescriptor` proto to be allocated on each call: - `sqlbase.MarshalColumnValue` - `sqlbase.CheckDatumTypeFitsColumnType` This showed up on IMPORT benchmarks, where the allocation in `MarshalColumnValue` was responsible for about a third of heap allocations. ``` name old time/op new time/op delta ImportFixtureTPCC-4 6.24s ± 2% 5.68s ±11% -9.02% (p=0.000 n=10+10) name old speed new speed delta ImportFixtureTPCC-4 14.2MB/s ± 2% 15.6MB/s ±10% +10.08% (p=0.000 n=10+10) name old alloc/op new alloc/op delta ImportFixtureTPCC-4 4.10GB ± 0% 2.77GB ± 0% -32.34% (p=0.000 n=10+10) name old allocs/op new allocs/op delta ImportFixtureTPCC-4 39.5M ± 0% 33.2M ± 0% -16.11% (p=0.000 n=10+9) ``` It appears that this will also have an effect on `row.Inserter` and `row.Updater`. Release note: None
c1e4605 to
b3f5776
Compare
Contributor
Author
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Mar 2, 2019
35317: sqlbase: avoid allocations due to ErrNameString, speed up IMPORT by 10% r=nvanbenschoten a=nvanbenschoten Informs #34809. Before this change, `ErrNameString` only accepted a pointer to a string. This forced the owner of the string to escape. In the following two cases, this resulted in an entire `ColumnDescriptor` proto to be allocated on each call: - `sqlbase.MarshalColumnValue` - `sqlbase.CheckDatumTypeFitsColumnType` This showed up on IMPORT benchmarks, where the allocation in `MarshalColumnValue` was responsible for about a third of heap allocations. ``` name old time/op new time/op delta ImportFixtureTPCC-4 6.24s ± 2% 5.68s ±11% -9.02% (p=0.000 n=10+10) name old speed new speed delta ImportFixtureTPCC-4 14.2MB/s ± 2% 15.6MB/s ±10% +10.08% (p=0.000 n=10+10) name old alloc/op new alloc/op delta ImportFixtureTPCC-4 4.10GB ± 0% 2.77GB ± 0% -32.34% (p=0.000 n=10+10) name old allocs/op new allocs/op delta ImportFixtureTPCC-4 39.5M ± 0% 33.2M ± 0% -16.11% (p=0.000 n=10+9) ``` It appears that this will also have an effect on `row.Inserter` and `row.Updater`. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Contributor
Build succeeded |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Informs #34809.
Before this change,
ErrNameStringonly accepted a pointer to a string.This forced the owner of the string to escape. In the following two
cases, this resulted in an entire
ColumnDescriptorproto to beallocated on each call:
sqlbase.MarshalColumnValuesqlbase.CheckDatumTypeFitsColumnTypeThis showed up on IMPORT benchmarks, where the allocation in
MarshalColumnValuewas responsible for about a third of heap allocations.
It appears that this will also have an effect on
row.Inserterandrow.Updater.Release note: None