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.

Sign up for GitHub

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

Jump to bottom

pgwire: too much work for serializing arrays #21711

Closed
andreimatei opened this issue Jan 23, 2018 · 7 comments · Fixed by #66941
Closed

pgwire: too much work for serializing arrays #21711

andreimatei opened this issue Jan 23, 2018 · 7 comments · Fixed by #66941
Labels
A-sql-pgwire pgwire protocol issues. C-performance Perf of queries or internals. Solution not expected to change functional behavior. good first issue T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
(DEPRECATED) SQL Front-end, Lang & Se... [DEPRECATED] Old SQLExec board. Don't... SQL Sessions - Deprecated

Comments

@andreimatei
Copy link
Contributor

We allocate a new writeBuffer every time we need to serialize an array. This seems very heavy. Can't we do something else? Like use the connections writeBuffer or keep around a specially built one?

cockroach/pkg/sql/pgwire/types.go

Line 391 in c4bd367

subWriter := newWriteBuffer()

@andreimatei andreimatei added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jan 23, 2018
@jordanlewis
Copy link
Member

Good point - did you notice any performance issues stemming from this, or did you just notice an inefficiency in the code?

@andreimatei
Copy link
Contributor Author

I was just trolling the code cause I'm changing the ctor for these buffers

@jordanlewis jordanlewis added this to the 2.1 milestone Feb 22, 2018
@knz
Copy link
Contributor

knz commented Apr 27, 2018

@jordanlewis what project do you reckon this belongs to?

@knz knz added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-sql-pgwire pgwire protocol issues. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Apr 27, 2018
@knz knz added this to Backlog in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Apr 27, 2018
@knz knz moved this from Triage to Backlog in (DEPRECATED) SQL Front-end, Lang & Semantics May 3, 2018
@knz knz modified the milestones: 2.1, 2.2 Oct 5, 2018
@petermattis petermattis removed this from the 2.2 milestone Oct 5, 2018
@jordanlewis jordanlewis moved this from Triage to Lower priority backlog in [DEPRECATED] Old SQLExec board. Don't move stuff here Apr 24, 2019
@asubiotto asubiotto moved this from Lower priority backlog to [TENT] SQL Features in [DEPRECATED] Old SQLExec board. Don't move stuff here Apr 2, 2020
@lamnguyen21
Copy link

@jordanlewis Is anyone working on this issue? If not, I would like to take on it as my first issue. 😄

@knz
Copy link
Contributor

knz commented Mar 16, 2021

go ahead.

@knz knz added this to Triage in SQL Sessions - Deprecated via automation Mar 16, 2021
@knz
Copy link
Contributor

knz commented Mar 16, 2021

cc @rafiss for re-triage

@rafiss
Copy link
Collaborator

rafiss commented Mar 16, 2021

@lamnguyen21 thanks for your interest!

some pointers:

  • familiarize yourself with BenchmarkEncodings in pkg/sql/pgwire/encoding_test.go. this tool should help you confirm if your changes are improving the number of allocations. you'll probably only want to focus on running the tuple and array benchmarks.
  • make sure your change updates both the tuple and array cases here:

    cockroach/pkg/sql/pgwire/types.go

    Line 447 in f5ed0e2

    subWriter := newWriteBuffer(nil /* bytecount */)
    and

    cockroach/pkg/sql/pgwire/types.go

    Line 480 in f5ed0e2

    subWriter := newWriteBuffer(nil /* bytecount */)
  • i think a fine approach here would be to keep around a special writeBuffer that is used specially for writing arrays and tuples. (note -- you need to have a separate one for each connection. maybe you could make it an internal field of the existing "actual" writeBuffer, but you'd want to add some safety guards around it. make sure to clear this "temp" writeBuffer out each time an array/tuple is moved over to the "actual" writeBuffer

@rafiss rafiss moved this from Triage to Smaller fixes/improvements in SQL Sessions - Deprecated Apr 28, 2021
@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
@biradarganesh25 biradarganesh25 mentioned this issue Jun 28, 2021
Merged
biradarganesh25 added a commit to biradarganesh25/cockroach that referenced this issue Jun 28, 2021
@biradarganesh25
pgwire: pre-allocate a temporary buffer to use when serializing array…
6c99ca3 
…s/tuples

A new "writeBuffer" was being allocated every time when serializing
an array/tuple. This is very heavy and unecessary.

To address this, this patch creates a temporary buffer in the
existing "writeBuffer" struct which is used specially for serializing
array/tuple. This is more efficient than allocating a new
temporary "writebuffer" every time.

Release note (performance improvement): A special temporary buffer was
created to be used only for serializing arrays/tuples. This provided
better performance. The increase in performance was seen by running the
BenchmarkEncodings benchmark for arrays and tuples.

Fixes: cockroachdb#21711
biradarganesh25 added a commit to biradarganesh25/cockroach that referenced this issue Jun 29, 2021
@biradarganesh25
pgwire: pre-allocate a temporary buffer to use when serializing array…
00a6a81 
…s/tuples

A new "writeBuffer" was being allocated every time when serializing
an array/tuple. This is very heavy and unecessary.

To address this, this patch creates a temporary buffer in the
existing "writeBuffer" struct which is used specially for serializing
array/tuple. This is more efficient than allocating a new
temporary "writebuffer" every time.

Release note (performance improvement): A special temporary buffer was
created to be used only for serializing arrays/tuples. This provided
better performance. The increase in performance was seen by running the
BenchmarkEncodings benchmark for arrays and tuples.

Fixes: cockroachdb#21711
biradarganesh25 added a commit to biradarganesh25/cockroach that referenced this issue Jun 30, 2021
@biradarganesh25
pgwire: pre-allocate a temporary buffer to use when serializing array…
ea1ab93 
…s/tuples

A new "writeBuffer" was being allocated every time when serializing
an array/tuple. This is very heavy and unecessary.

To address this, this patch creates a temporary buffer in the
existing "writeBuffer" struct which is used specially for serializing
array/tuple. This is more efficient than allocating a new
temporary "writebuffer" every time.

Release note (performance improvement): A special temporary buffer was
created to be used only for serializing arrays/tuples. This provided
better performance. The increase in performance was seen by running the
BenchmarkEncodings benchmark for arrays and tuples.

Fixes: cockroachdb#21711
biradarganesh25 added a commit to biradarganesh25/cockroach that referenced this issue Jun 30, 2021
@biradarganesh25
pgwire: pre-allocate a temporary buffer to use when serializing array…
d777e8e 
…s/tuples

A new "writeBuffer" was being allocated every time when serializing
an array/tuple. This is very heavy and unecessary.

To address this, this patch creates a temporary buffer in the
existing "writeBuffer" struct which is used specially for serializing
array/tuple. This is more efficient than allocating a new
temporary "writebuffer" every time.

Release note (performance improvement): A special temporary buffer was
created to be used only for serializing arrays/tuples. This provided
better performance. The increase in performance was seen by running the
BenchmarkEncodings benchmark for arrays and tuples.

Fixes: cockroachdb#21711
@craig craig bot closed this as completed in 2ed3c02 Jul 19, 2021
SQL Sessions - Deprecated automation moved this from Smaller fixes/improvements to Done Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-sql-pgwire pgwire protocol issues. C-performance Perf of queries or internals. Solution not expected to change functional behavior. good first issue T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
(DEPRECATED) SQL Front-end, Lang & Se...
  
Backlog
SQL Sessions - Deprecated
Done
[DEPRECATED] Old SQLExec board. Don't...
SQL Features Backlog
Milestone
No milestone
6 participants
@jordanlewis @andreimatei @knz @rafiss @petermattis @lamnguyen21