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

perf: replace BufferedOutputStream with unsynchronized PgBufferedOutputStream, allow configuring different Java and SO_SNDBUF buffer sizes #3248

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented May 16, 2024

This fixes two issues:

  1. Java 21 uses ReentrantLock for BufferedOutputStream, however, we do not need synchronization as we synchronize at the level of QueryExecutor.execute

  2. The default buffer size of 8192 was small for the modern connections

Fixes #3245

TODO:

  • Benchmark large reads and writes
  • Add extra tests for PgBufferedOutputStream
  • Add new property maxSendBufferSize
  • Move PGStream.sendInteger4 and PGStream.sendInteger2 to PgBufferedOutputStream so we do not need intermediate int[4] arrays for sending integers
  • Move PGStream.sendStream to PgBufferedOutputStream so we can reuse PgBufferedOutputStream's buffer when reading data from the InputStream
  • Refactor GSSOutputStream so it limits the produced messages (backend limits message size with 16KiB)
  • Refactor GSSOutputStream so it avoids intermediate copies of the data when writing large array to GSS stream

Cancelled:

  • Modify default value for send buffer size
  • Modify receive buffer size
  • Dynamically switch to a larger buffer size if we know there will be a big send request? As we send messages we do determine the message size in advance, so we can proactively increase the send buffer in case we know there will be many small writes (e.g. many binds)

/cc @plokhotnyuk

@plokhotnyuk
Copy link

The profile shows even better numbers than in my closed PR:
image

@vlsi
Copy link
Member Author

vlsi commented May 17, 2024

Just wondering: do you have org/postgresql/core/v3/QueryExecutorImpl.sendParse on the profile?

@plokhotnyuk
Copy link

plokhotnyuk commented May 17, 2024

Just wondering: do you have org/postgresql/core/v3/QueryExecutorImpl.sendParse on the profile?

Yes, it is in the right corner of that profile with 56 samples:
image

@vlsi
Copy link
Member Author

vlsi commented May 17, 2024

@plokhotnyuk , could you please try commenting this line

flags |= QueryExecutor.QUERY_ONESHOT;
?

Would it alleviate QueryExecutorImpl.sendParse in your workload?

@plokhotnyuk
Copy link

@plokhotnyuk , could you please try commenting this line

flags |= QueryExecutor.QUERY_ONESHOT;

?

Would it alleviate QueryExecutorImpl.sendParse in your workload?

I'm using PgPreparedStatement for the batch query and most CPU cycles of sendParse are burned in the isPrepareFor check:
image

@vlsi vlsi force-pushed the buffer_size branch 2 times, most recently from 8e7b129 to 91388c3 Compare May 19, 2024 06:56
@vlsi vlsi changed the title perf: replace BufferedOutputStream with unsyncrhonized PgBufferedOutputStream, increase the send buffer size perf: replace BufferedOutputStream with unsynchronized PgBufferedOutputStream, increase the send buffer size May 19, 2024
@vlsi vlsi force-pushed the buffer_size branch 7 times, most recently from 7c02456 to 5a76511 Compare May 19, 2024 08:14
@vlsi
Copy link
Member Author

vlsi commented May 19, 2024

I noticed there's PGStream.streamBuffer byte[8192] which we use for sendStream(InputStream inStream).

It looks like we could add PgBufferedStream.write(InputStream) so we reuse and buffer from PgBufferedStream.
Any thoughts regarding that?

@plokhotnyuk
Copy link

Do we have any chance to re-use byte array buffers of PgBufferedStream to avoid redundant copying through int4, int2 buffers?

But before that need to find an ability to avoid boxing when checking type of serialisation for oids using generic hash maps.

@vlsi
Copy link
Member Author

vlsi commented May 19, 2024

Do we have any chance to re-use byte array buffers of PgBufferedStream to avoid redundant copying through int4, int2 buffers?

This PR already fixes it for output, see PgBufferedOutputStream.writeInt2 and PgBufferedOutputStream.writeInt4.

But before that need to find an ability to avoid boxing when checking type of serialisation for oids using generic hash maps

That is on my list. Try using BitSet. I just did not want to mix many changes into a single PR.
The following improves useBinaryForSend for me:

@@ -62,6 +62,7 @@ import java.sql.SQLException;
 import java.sql.SQLWarning;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.BitSet;
 import java.util.Collections;
 import java.util.Deque;
 import java.util.HashMap;
@@ -128,6 +129,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
    * Bit set that has a bit set for each oid which should be sent using binary format.
    */
   private final Set<Integer> useBinarySendForOids = new HashSet<>();
+  private final BitSet useBinarySendForOidsBs = new BitSet();

   /**
    * This is a fake query object so processResults can distinguish "ReadyForQuery" messages
@@ -3005,6 +3007,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
   public void addBinarySendOid(int oid) {
     synchronized (useBinarySendForOids) {
       useBinarySendForOids.add(oid);
+      useBinarySendForOidsBs.set(oid);
     }
   }

@@ -3012,6 +3015,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
   public void removeBinarySendOid(int oid) {
     synchronized (useBinarySendForOids) {
       useBinarySendForOids.remove(oid);
+      useBinarySendForOidsBs.set(oid, false);
     }
   }

@@ -3027,7 +3031,8 @@ public class QueryExecutorImpl extends QueryExecutorBase {
   @Override
   public boolean useBinaryForSend(int oid) {
     synchronized (useBinarySendForOids) {
-      return useBinarySendForOids.contains(oid);
+      return useBinarySendForOidsBs.get(oid);
     }
   }

@@ -3036,6 +3041,9 @@ public class QueryExecutorImpl extends QueryExecutorBase {
     synchronized (useBinarySendForOids) {
       useBinarySendForOids.clear();
       useBinarySendForOids.addAll(oids);
+      for (Integer oid : oids) {
+        useBinarySendForOidsBs.set(oid);
+      }
     }
   }

@vlsi vlsi force-pushed the buffer_size branch 3 times, most recently from 3c6412a to 61a6b1c Compare May 19, 2024 13:19
@vlsi
Copy link
Member Author

vlsi commented May 19, 2024

The changes look good to me. It is still unclear if socket.getSendBufferSize() is a safe default.

vlsi added 2 commits May 19, 2024 16:31
…utStream, increase the send buffer size

This fixes the following issues:
1) Java 21 uses ReentrantLock for BufferedOutputStream, however, we do not need synchronization
as we synchronize at the level of QueryExecutor.execute

2) The default buffer size of 8192 was small for the modern connections

3) The previous implementation of GSSOutputStream could produce oversize packets
that would be rejected by the backend.


Fixes pgjdbc#3245

See https://github.com/postgres/postgres/blob/acecd6746cdc2df5ba8dcc2c2307c6560c7c2492/src/backend/libpq/be-secure-gssapi.c#L348
@vlsi
Copy link
Member Author

vlsi commented May 20, 2024

@plokhotnyuk , do you think you could benchmark 32KiB, 64KiB, 128KiB heap buffer sizes?
I wonder if increasing the heap buffer after 64KiB produces any improvements.

I am leaning towards adding sendMaxHeapBufferSize and receiveMaxHeapBufferSize and set them to 64KiB by default.

@plokhotnyuk
Copy link

plokhotnyuk commented May 20, 2024

@plokhotnyuk , do you think you could benchmark 32KiB, 64KiB, 128KiB heap buffer sizes? I wonder if increasing the heap buffer after 64KiB produces any improvements.

I am leaning towards adding sendMaxHeapBufferSize and receiveMaxHeapBufferSize and set them to 64KiB by default.

I think that having those options configurable even with setting them to 8KiB by default as for the current latest release would be an acceptable step, because bigger buffers will be required for batches mostly.

Comment on lines +193 to +195
// Cancel signal is 16 bytes only, so we use 16 as the max send buffer size
cancelStream =
new PGStream(pgStream.getSocketFactory(), pgStream.getHostSpec(), cancelSignalTimeout);
new PGStream(pgStream.getSocketFactory(), pgStream.getHostSpec(), cancelSignalTimeout, 16);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the only place where we can justify the buffer size :)

@vlsi vlsi changed the title perf: replace BufferedOutputStream with unsynchronized PgBufferedOutputStream, increase the send buffer size perf: replace BufferedOutputStream with unsynchronized PgBufferedOutputStream, allow configuring different Java and SO_SNDBUF buffer sizes May 20, 2024
@plokhotnyuk
Copy link

@bokken Could you please review this PR to be merged and released in the next version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants