ByteBufBsonDocument remains a ByteBuf #1119

jyemin · 2023-05-17T18:50:37Z

This one got a bit complicated. The root cause of the bug is that ByteBufBsonDocument#entrySet hydrates its byte buffer into the base class's hash map instead of implementing the entry set via iterating over the raw BSON. To avoid this hydration completely, including for nested documents and arrays, requires quite a few code changes, including the introduction of a new class, ByteBufBsonArray, that does the same job for nested arrays.

JAVA-4917

* Ensure that ByteBufBsonDocument never hydrates itself into a HashMap * Add ByteBufBsonArray and ensure it never hydrates itself into an ArrayList JAVA-4917

jyemin · 2023-05-22T13:09:29Z

bson/src/main/org/bson/BsonArray.java

@@ -237,7 +237,7 @@ public int hashCode() {
    @Override
    public String toString() {
        return "BsonArray{"
-               + "values=" + values


To allow subclasses to override #getValues

jyemin · 2023-05-22T13:10:08Z

driver-core/src/main/com/mongodb/internal/connection/AbstractByteBufBsonDocument.java

To simplify the design, this class was merged into the only actual subclass, `ByteBufBsonDocument``

jyemin · 2023-05-22T13:19:09Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonArray.java

+
+import static com.mongodb.internal.connection.ByteBufBsonHelper.readBsonValue;
+
+public class ByteBufBsonArray extends BsonArray {


A lot of the implementation of the overridden methods in this class were adapted from the JDKs abstract collection classes. Unfortunately, we can't just call the existing methods directly due to the fact that we have to extend BsonArray rather than, say, AbstractList. This is due to a design flaw in the driver. Really, BsonArray and BsonDocument should have been interfaces, but because they are classes we are forced to extend them rather than implement them, and the subclasses (like this one) have to do a lot of work to ensure that the base class implementations are not used (effectively treating them as interfaces or pure abstract classes.

jyemin · 2023-05-22T13:34:41Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonArray.java

+
+    @Override
+    public ListIterator<BsonValue> listIterator(final int index) {
+        // Not the most efficient way to do this, but unlikely anyone will notice in practice


I don't think it's worth investing in a more efficient ListIterator implementation. Ultimately, it's going to have to hydrate the array, so it's not going to be much better than this for most use cases.

jyemin · 2023-05-22T13:36:39Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonDocument.java

    private static final long serialVersionUID = 2L;

    private final transient ByteBuf byteBuf;

-    static List<ByteBufBsonDocument> createList(final ResponseBuffers responseBuffers) {


Unused method

jyemin · 2023-05-22T13:37:59Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonDocument.java

+        byteBuf.get(byteBuf.position(), clonedBytes);
+        return new RawBsonDocument(clonedBytes);
+    }
+
    @Nullable
    <T> T findInDocument(final Finder<T> finder) {
        ByteBuf duplicateByteBuf = byteBuf.duplicate();


The double duplication of the byte buf was unintentional, but caused a bug once the hydration behavior was changed

jyemin · 2023-05-22T13:38:24Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonDocument.java

            bsonReader.readStartDocument();
            while (bsonReader.readBsonType() != BsonType.END_OF_DOCUMENT) {
-                T found = finder.find(bsonReader);
+                T found = finder.find(duplicateByteBuf, bsonReader);


Now we need the byte buf sent to the finder to avoid hydration of nested documents/arrays

vbabanin · 2023-06-07T19:28:43Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonArray.java

+        return values;
+    }
+
+    private static final String READ_ONLY_MESSAGE = "This BsonArray instance is read-only";


nit: static final can be moved to the top of the class

vbabanin · 2023-06-07T19:28:52Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonArray.java

+
+    @Override
+    public int size() {
+        int size = 0;


Since the ByteBufBsonArray is read-only and its size doesn't change after initialization, we can consider storing the size value after first call to avoid additional computations during each size() call.

It's true, and I did consider a few optimizations like this but decided to keep it simple.

rozza

LGTM

Could / should RawBsonDocument now delegate to ByteBufBsonDocument internally?

Would save two very similar document types.

rozza · 2023-06-08T15:23:41Z

driver-core/src/main/com/mongodb/internal/connection/ByteBufBsonArray.java

+    @SuppressWarnings("unchecked")
+    public <T> T[] toArray(final T[] a) {
+        int size = size();
+        T[] retVal = a.length >= size ? a : (T[]) java.lang.reflect.Array.newInstance(a.getClass().getComponentType(), size);


Wow - that's gnarly

jyemin · 2023-06-13T12:44:47Z

Could / should RawBsonDocument now delegate to ByteBufBsonDocument internally

Added JAVA-5023 to consider this

jyemin self-assigned this May 17, 2023

jyemin force-pushed the j4917 branch 5 times, most recently from 3d439c8 to 183edf6 Compare May 22, 2023 02:40

jyemin changed the title ~~Command logs don't reflect actual command/reply documents~~ ByteBufBsonDocument remains a ByteBuf May 22, 2023

jyemin force-pushed the j4917 branch from 183edf6 to a341291 Compare May 22, 2023 13:44

ByteBufBsonDocument remains a ByteBuf

e0af16d

* Ensure that ByteBufBsonDocument never hydrates itself into a HashMap * Add ByteBufBsonArray and ensure it never hydrates itself into an ArrayList JAVA-4917

jyemin force-pushed the j4917 branch from a341291 to e0af16d Compare June 1, 2023 14:26

jyemin commented Jun 1, 2023

View reviewed changes

jyemin requested review from stIncMale and vbabanin June 1, 2023 14:27

jyemin marked this pull request as ready for review June 1, 2023 14:28

vbabanin reviewed Jun 7, 2023

View reviewed changes

vbabanin approved these changes Jun 7, 2023

View reviewed changes

stIncMale requested review from rozza and removed request for stIncMale June 8, 2023 14:38

rozza approved these changes Jun 13, 2023

View reviewed changes

jyemin merged commit 2468e98 into mongodb:master Jun 13, 2023

jyemin deleted the j4917 branch June 13, 2023 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ByteBufBsonDocument remains a ByteBuf #1119

ByteBufBsonDocument remains a ByteBuf #1119

jyemin commented May 17, 2023 •

edited

Loading

jyemin May 22, 2023

jyemin May 22, 2023

jyemin May 22, 2023

jyemin May 22, 2023

jyemin May 22, 2023

jyemin May 22, 2023

jyemin May 22, 2023

vbabanin Jun 7, 2023

vbabanin Jun 7, 2023 •

edited

Loading

jyemin Jun 13, 2023

rozza left a comment

rozza Jun 8, 2023

jyemin commented Jun 13, 2023


		import static com.mongodb.internal.connection.ByteBufBsonHelper.readBsonValue;

		public class ByteBufBsonArray extends BsonArray {

ByteBufBsonDocument remains a ByteBuf #1119

ByteBufBsonDocument remains a ByteBuf #1119

Conversation

jyemin commented May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbabanin Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin commented Jun 13, 2023

jyemin commented May 17, 2023 •

edited

Loading

vbabanin Jun 7, 2023 •

edited

Loading