Using synchronized(this) instead of InternalLock when writing to sockets

[plokhotnyuk]

Currently when inserting with rewritten batches a lot of CPU is spent on lock()/unlock() calls of BufferedOutputStream on writing of each value.

Proposed changes switch internal implementation of BufferedOutputStream to use syncronized(this) instead of InternalLock

If synchronization is not needed then a better approach would be using of custom implementation.

[vlsi]

Could you please share CPU profiles?

[plokhotnyuk]

Before:

After:

Also, it would be good to add an ability to configure size of this buffer up to 64Kb for yet more efficient working with batches.

Here is a CPU profile with this change and 128k buffer:

[vlsi]

I believe the synchronization is not required there since we already synchronize via QueryExecutorImpl.lock.

At the same time, if we go with "regular synchronized", then it would become loom-incompatible as flushBuffer would happen under a lock, and it would call IO.

WDYT of creating an unsynchronized BufferedOutputStream?

[plokhotnyuk]

I believe the synchronization is not required there since we already synchronize via QueryExecutorImpl.lock.

At the same time, if we go with "regular synchronized", then it would become loom-incompatible as flushBuffer would happen under a lock, and it would call IO.

WDYT of creating an unsynchronized BufferedOutputStream?

I think it will be the best option but I have no idea how to allow user to tune its max and initial sizes

[davecramer]

I'm wondering if there are other problems like this ?
Either way. If we are locking in QueryExecutorImpl then it seems fine to remove the lock from BufferOutputStream

[vlsi]

how to allow user to tune its max and initial sizes

We have sendBufferSize connection property. What do you think if we use the same value for the buffer size?
If the value is -1, then use getSendBufferSize() on the connection and use something like Math.min(8192, getSendBufferSize()).

[vlsi]

I'm wondering if there are other problems like this ?

I guess we could run into similar issues if we add lock-based synchronization in methods like PreparedStatement.setString.
The current issue is visible since we have a lot of calls like pgStream.sendInteger4 as we have one call for every value we send. That generates a lot of buffer.write calls which trigger a lot of InternalLock operations.

[davecramer]

how to allow user to tune its max and initial sizes

We have sendBufferSize connection property. What do you think if we use the same value for the buffer size? If the value is -1, then use getSendBufferSize() on the connection and use something like Math.min(8192, getSendBufferSize()).

If the value of what is -1 ?

[plokhotnyuk]

how to allow user to tune its max and initial sizes

We have sendBufferSize connection property. What do you think if we use the same value for the buffer size? If the value is -1, then use getSendBufferSize() on the connection and use something like Math.min(8192, getSendBufferSize()).

Do you mean getSocket().getSendBufferSize() when called in PGStream.changeSocket(Socket socket) implementation?

[vlsi]

If the value of what is -1 ?

I have no idea what socket.getSendBufferSize() can return, however, if it returns something like 1024, I would still prefer buffering 8192 in the heap.

--

I've just tested, and macOS 14.4.1 yield getSendBufferSize() of 146988 when executed with Java 8, 11, 17, and 23.

So if we go with getSendBufferSize(), then it might significantly increase per-connection heap requirements (e.g. 100 connections would consume ~150 megabytes, while currently it is just 8).
That sounds like a showstopper to using getSendBufferSize() as a default buffer size.

[vlsi]

My calculations were off in the previous comment.

If a default buffer size is 150KiB, then 100 connections would consume 15MiB which is more-or-less reasonable to have by default.

[vlsi]

By the way, the backend uses 8192 buffer size: https://github.com/postgres/postgres/blob/3ddbac368c205fce1f293de1fe60c1b479800746/src/backend/libpq/pqcomm.c#L118-L119

See https://www.postgresql.org/message-id/0cdc5485-cb3c-5e16-4a46-e3b2f7a41322%40ya.ru

However, they mention the performance increase is not significant for 1Gbps links: https://www.postgresql.org/message-id/20190728202141.GA25242%40hjp.at

[plokhotnyuk]

By the way, the backend uses 8192 buffer size: https://github.com/postgres/postgres/blob/3ddbac368c205fce1f293de1fe60c1b479800746/src/backend/libpq/pqcomm.c#L118-L119

See https://www.postgresql.org/message-id/0cdc5485-cb3c-5e16-4a46-e3b2f7a41322%40ya.ru

However, they mention the performance increase is not significant for 1Gbps links: https://www.postgresql.org/message-id/20190728202141.GA25242%40hjp.at

Times of 1Gbit LANs are gone, contemporary server LANs are 10-100Gbit.

Our kernel settings for read/write buffers:

net.core.rmem_max = 4194304
net.core.rmem_default = 65536
net.core.wmem_max = 4194304
net.core.wmem_default = 65536

But for batched inserts/updates we use localhost connections to reduce saturation of network bandwidth ;)

[plokhotnyuk]

Closing in favor of #3248