You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/10/11 15:18:17 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request, #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
ibessonov opened a new pull request, #1189:
URL: https://github.com/apache/ignite-3/pull/1189
https://issues.apache.org/jira/browse/IGNITE-17874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993217255
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -65,6 +66,18 @@ public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
/** Poison object. */
private static final Object NULL = new Object();
+ /** Flag that indicates that byte buffer is not null. */
+ private static final byte BYTE_BYFFER_NOT_NULL_FLAG = 1;
Review Comment:
There's a typo in all of these constants, it's `BYTE_BUFFER`, not `BYTE_BYFFER`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993075483
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
+ }
+
+ if (val.limit() == val.capacity()) {
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993230147
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +517,89 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= BYTE_BYFFER_NOT_NULL_FLAG;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= BYTE_BYFFER_BIG_ENDIAN_FLAG;
+ }
+
+ if (val.position() != 0) {
+ flag |= BYTE_BYFFER_HAS_POSITION_FLAG;
+ }
+
+ if (val.limit() != val.capacity()) {
+ flag |= BYTE_BYFFER_HAS_LIMIT_FLAG;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if (val.position() != 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if (val.limit() != val.capacity()) {
+ writeInt(val.limit());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ assert !val.isReadOnly();
+
+ int length = val.capacity();
+
+ if (val.isDirect()) {
+ long address = GridUnsafe.bufferAddress(val);
+
+ lastFinished = writeArray(null, address, length, length);
Review Comment:
This way I reused code from writeArray, otherwise I'd have to copy it, basically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993227785
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1363,8 +1541,7 @@ public <C extends Collection<?>> C readCollection(MessageCollectionItemType item
* @return Whether array was fully written.
*/
boolean writeArray(Object arr, long off, int len, int bytes) {
- assert arr != null;
- assert arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
+ assert arr == null || arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
Review Comment:
It was, I pass "null" here for direct buffers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993221995
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageCollectionItemTypeConverter.java:
##########
@@ -125,6 +126,8 @@ private MessageCollectionItemType fromDeclaredType(DeclaredType parameterType) {
return MessageCollectionItemType.MSG;
} else if (typeUtils.isSameType(parameterType, BitSet.class)) {
return MessageCollectionItemType.BIT_SET;
+ } else if (typeUtils.isSameType(parameterType, ByteBuffer.class)) {
Review Comment:
ok, fair
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993076089
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1049,6 +1166,80 @@ public BitSet readBitSet() {
return arr != null ? BitSet.valueOf(arr) : null;
}
+ @Override
+ public ByteBuffer readByteBuffer() {
+ byte[] bytes;
+
+ switch (byteBufferState) {
+ case 0:
+ byteBufferFlag = readByte();
+
+ boolean isNull = byteBufferFlag == 0;
+
+ if (!lastFinished || isNull) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & 4) != 0) {
+ byteBufferPosition = 0;
+ } else {
+ byteBufferPosition = readInt();
+ }
+
+ if (!lastFinished) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if ((byteBufferFlag & 8) != 0) {
+ byteBufferLimit = -1;
+ } else {
+ byteBufferLimit = readInt();
+ }
+
+ if (!lastFinished) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ bytes = readByteArray();
+
+ if (!lastFinished ) {
+ return null;
+ }
+
+ byteBufferState = 0;
+ break;
+
+ default:
+ throw new IllegalArgumentException("Unknown byteBufferState: " + uuidState);
+ }
+
+ ByteBuffer val = ByteBuffer.wrap(bytes).position(byteBufferPosition);
Review Comment:
This is intended, we'll always create heap buffers to avoid problems. Or should we change it? "allocateDirect" should be avoided IMO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993075269
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
+ }
+
+ if (val.limit() == val.capacity()) {
+ flag |= 8;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & 4) == 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if ((byteBufferFlag & 8) != 0) {
+ writeInt(val.limit());
Review Comment:
Bug...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993230147
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +517,89 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= BYTE_BYFFER_NOT_NULL_FLAG;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= BYTE_BYFFER_BIG_ENDIAN_FLAG;
+ }
+
+ if (val.position() != 0) {
+ flag |= BYTE_BYFFER_HAS_POSITION_FLAG;
+ }
+
+ if (val.limit() != val.capacity()) {
+ flag |= BYTE_BYFFER_HAS_LIMIT_FLAG;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if (val.position() != 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if (val.limit() != val.capacity()) {
+ writeInt(val.limit());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ assert !val.isReadOnly();
+
+ int length = val.capacity();
+
+ if (val.isDirect()) {
+ long address = GridUnsafe.bufferAddress(val);
+
+ lastFinished = writeArray(null, address, length, length);
Review Comment:
This way I reused code from writeArray, otherwise I'd have to copy it, basically. It does more than just copying, it also manages last position, finished flag and so on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov merged pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov merged PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993076397
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMessageReader.java:
##########
@@ -371,6 +383,20 @@ public <C extends Collection<?>> C readCollection(String name, MessageCollection
return col;
}
+ /** {@inheritDoc} */
+ @Override
+ public <C extends List<?>> C readList(String name, MessageCollectionItemType itemType) {
+ DirectByteBufferStream stream = state.item().stream;
+
+ Collection<?> col = stream.readCollection(itemType, this);
+
+ lastRead = stream.lastFinished();
+
+ assert col instanceof List;
Review Comment:
You told me that, when we read them, we always create a list
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993232903
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +517,89 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= BYTE_BYFFER_NOT_NULL_FLAG;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= BYTE_BYFFER_BIG_ENDIAN_FLAG;
+ }
+
+ if (val.position() != 0) {
+ flag |= BYTE_BYFFER_HAS_POSITION_FLAG;
+ }
+
+ if (val.limit() != val.capacity()) {
+ flag |= BYTE_BYFFER_HAS_LIMIT_FLAG;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if (val.position() != 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if (val.limit() != val.capacity()) {
+ writeInt(val.limit());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ assert !val.isReadOnly();
+
+ int length = val.capacity();
+
+ if (val.isDirect()) {
+ long address = GridUnsafe.bufferAddress(val);
+
+ lastFinished = writeArray(null, address, length, length);
+ } else {
+ byte[] array = val.array();
Review Comment:
Would be weird, imo. What if receiver wants to set position to 0? These two numbers are just extra values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993074504
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMarshallingUtils.java:
##########
@@ -31,20 +31,31 @@ public class DirectMarshallingUtils {
* @return Direct message type.
*/
public static short getShort(ByteBuffer buffer) {
- byte b0 = buffer.get();
- byte b1 = buffer.get();
+ int primShift = 0;
+ int prim = 0;
Review Comment:
I'm removing this code, too many problems with it, we'll do it later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993074277
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageCollectionItemTypeConverter.java:
##########
@@ -125,6 +126,8 @@ private MessageCollectionItemType fromDeclaredType(DeclaredType parameterType) {
return MessageCollectionItemType.MSG;
} else if (typeUtils.isSameType(parameterType, BitSet.class)) {
return MessageCollectionItemType.BIT_SET;
+ } else if (typeUtils.isSameType(parameterType, ByteBuffer.class)) {
Review Comment:
Who would use a subtype of byte buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993231258
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1363,8 +1541,7 @@ public <C extends Collection<?>> C readCollection(MessageCollectionItemType item
* @return Whether array was fully written.
*/
boolean writeArray(Object arr, long off, int len, int bytes) {
- assert arr != null;
- assert arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
+ assert arr == null || arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
Review Comment:
Ok, didn't notice that, thanks for the explanation. Please add `@Nullable` here, then
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r992754010
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
Review Comment:
I guess you wanted to use `byteBufferFlag` here? Or I don't get it
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
Review Comment:
I'd personally prefer it inversed. Setting the flag if position is not zero
##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/serialization/MessageCollectionItemTypeConverter.java:
##########
@@ -125,6 +126,8 @@ private MessageCollectionItemType fromDeclaredType(DeclaredType parameterType) {
return MessageCollectionItemType.MSG;
} else if (typeUtils.isSameType(parameterType, BitSet.class)) {
return MessageCollectionItemType.BIT_SET;
+ } else if (typeUtils.isSameType(parameterType, ByteBuffer.class)) {
Review Comment:
Shouldn't it be `isSubType`?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMarshallingUtils.java:
##########
@@ -31,20 +31,31 @@ public class DirectMarshallingUtils {
* @return Direct message type.
*/
public static short getShort(ByteBuffer buffer) {
- byte b0 = buffer.get();
- byte b1 = buffer.get();
+ int primShift = 0;
+ int prim = 0;
Review Comment:
What is a prim?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1049,6 +1166,80 @@ public BitSet readBitSet() {
return arr != null ? BitSet.valueOf(arr) : null;
}
+ @Override
+ public ByteBuffer readByteBuffer() {
+ byte[] bytes;
+
+ switch (byteBufferState) {
+ case 0:
+ byteBufferFlag = readByte();
+
+ boolean isNull = byteBufferFlag == 0;
+
+ if (!lastFinished || isNull) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & 4) != 0) {
+ byteBufferPosition = 0;
+ } else {
+ byteBufferPosition = readInt();
+ }
+
+ if (!lastFinished) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if ((byteBufferFlag & 8) != 0) {
+ byteBufferLimit = -1;
Review Comment:
This seems to be correct, but in `write` method we write `limit` in case if flag is set
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/DirectMessageReader.java:
##########
@@ -371,6 +383,20 @@ public <C extends Collection<?>> C readCollection(String name, MessageCollection
return col;
}
+ /** {@inheritDoc} */
+ @Override
+ public <C extends List<?>> C readList(String name, MessageCollectionItemType itemType) {
+ DirectByteBufferStream stream = state.item().stream;
+
+ Collection<?> col = stream.readCollection(itemType, this);
+
+ lastRead = stream.lastFinished();
+
+ assert col instanceof List;
Review Comment:
Hm, so all our collections are always lists?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1049,6 +1166,80 @@ public BitSet readBitSet() {
return arr != null ? BitSet.valueOf(arr) : null;
}
+ @Override
+ public ByteBuffer readByteBuffer() {
+ byte[] bytes;
+
+ switch (byteBufferState) {
+ case 0:
+ byteBufferFlag = readByte();
+
+ boolean isNull = byteBufferFlag == 0;
+
+ if (!lastFinished || isNull) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & 4) != 0) {
+ byteBufferPosition = 0;
+ } else {
+ byteBufferPosition = readInt();
+ }
+
+ if (!lastFinished) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if ((byteBufferFlag & 8) != 0) {
+ byteBufferLimit = -1;
+ } else {
+ byteBufferLimit = readInt();
+ }
+
+ if (!lastFinished) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ bytes = readByteArray();
+
+ if (!lastFinished ) {
+ return null;
+ }
+
+ byteBufferState = 0;
+ break;
+
+ default:
+ throw new IllegalArgumentException("Unknown byteBufferState: " + uuidState);
+ }
+
+ ByteBuffer val = ByteBuffer.wrap(bytes).position(byteBufferPosition);
Review Comment:
It's always a heap buffer, seems odd
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
+ }
+
+ if (val.limit() == val.capacity()) {
Review Comment:
I'd prefer != here. So if limit is set, then set the flag
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
+ }
+
+ if (val.limit() == val.capacity()) {
+ flag |= 8;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & 4) == 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if ((byteBufferFlag & 8) != 0) {
+ writeInt(val.limit());
Review Comment:
So we write `limit` if it is equal to the capacity? Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993212122
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +517,89 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= BYTE_BYFFER_NOT_NULL_FLAG;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= BYTE_BYFFER_BIG_ENDIAN_FLAG;
+ }
+
+ if (val.position() != 0) {
+ flag |= BYTE_BYFFER_HAS_POSITION_FLAG;
+ }
+
+ if (val.limit() != val.capacity()) {
+ flag |= BYTE_BYFFER_HAS_LIMIT_FLAG;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if (val.position() != 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if (val.limit() != val.capacity()) {
+ writeInt(val.limit());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ assert !val.isReadOnly();
+
+ int length = val.capacity();
+
+ if (val.isDirect()) {
+ long address = GridUnsafe.bufferAddress(val);
+
+ lastFinished = writeArray(null, address, length, length);
Review Comment:
I don't understand, why do we bother with all this GridUnsafe shenanigans when writing an array or a ByteBuffer into another ByteBuffer. What's wrong with simply calling `put(ByteBuffer)`?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -65,6 +66,18 @@ public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
/** Poison object. */
private static final Object NULL = new Object();
+ /** Flag that indicates that byte buffer is not null. */
+ private static final byte BYTE_BYFFER_NOT_NULL_FLAG = 1;
Review Comment:
There's a typo in all these constants, its `BYTE_BUFFER`, not `BYTE_BYFFER`
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1049,6 +1153,80 @@ public BitSet readBitSet() {
return arr != null ? BitSet.valueOf(arr) : null;
}
+ @Override
+ public ByteBuffer readByteBuffer() {
+ byte[] bytes;
+
+ switch (byteBufferState) {
+ case 0:
+ byteBufferFlag = readByte();
+
+ boolean isNull = (byteBufferFlag & BYTE_BYFFER_NOT_NULL_FLAG) == 0;
+
+ if (!lastFinished || isNull) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & BYTE_BYFFER_HAS_POSITION_FLAG) == 0) {
+ byteBufferPosition = 0;
+ } else {
+ byteBufferPosition = readInt();
+ }
+
+ if (!lastFinished) {
Review Comment:
maybe this `if` should be moved to closer to the `readInt` call
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1363,8 +1541,7 @@ public <C extends Collection<?>> C readCollection(MessageCollectionItemType item
* @return Whether array was fully written.
*/
boolean writeArray(Object arr, long off, int len, int bytes) {
- assert arr != null;
- assert arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
+ assert arr == null || arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
Review Comment:
I don't think this change was necessary
##########
modules/network/src/test/java/org/apache/ignite/internal/network/messages/AllTypesMessage.java:
##########
@@ -78,4 +80,8 @@ public interface AllTypesMessage extends NetworkMessage, Serializable {
Collection<NetworkMessage> netMsgCollW();
Map<String, NetworkMessage> newMsgMapX();
+
+ List<NetworkMessage> netMsgListY();
Review Comment:
Not related to this PR, but theres' a typo above: `newMsgMapX`
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +517,89 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= BYTE_BYFFER_NOT_NULL_FLAG;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= BYTE_BYFFER_BIG_ENDIAN_FLAG;
+ }
+
+ if (val.position() != 0) {
+ flag |= BYTE_BYFFER_HAS_POSITION_FLAG;
+ }
+
+ if (val.limit() != val.capacity()) {
+ flag |= BYTE_BYFFER_HAS_LIMIT_FLAG;
+ }
+ }
+
+ writeByte(flag);
+
+ if (!lastFinished || val == null) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if (val.position() != 0) {
+ writeInt(val.position());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 2:
+ if (val.limit() != val.capacity()) {
+ writeInt(val.limit());
+ }
+
+ if (!lastFinished) {
+ return;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 3:
+ assert !val.isReadOnly();
+
+ int length = val.capacity();
+
+ if (val.isDirect()) {
+ long address = GridUnsafe.bufferAddress(val);
+
+ lastFinished = writeArray(null, address, length, length);
+ } else {
+ byte[] array = val.array();
Review Comment:
hm, didn't we agree that we should transfer only (limit - position) bytes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993074882
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
Review Comment:
No, local variable is intended. I'll refactor it a little bit, it'll get better
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -496,6 +513,85 @@ public void writeBitSet(BitSet val) {
writeLongArray(val != null ? val.toLongArray() : null);
}
+ /** {@inheritDoc} */
+ @Override
+ public void writeByteBuffer(ByteBuffer val) {
+ switch (byteBufferState) {
+ case 0:
+ byte flag = 0;
+
+ if (val != null) {
+ flag |= 1;
+
+ if (val.order() == ByteOrder.BIG_ENDIAN) {
+ flag |= 2;
+ }
+
+ if (val.position() == 0) {
+ flag |= 4;
Review Comment:
Ok, why not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993231258
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1363,8 +1541,7 @@ public <C extends Collection<?>> C readCollection(MessageCollectionItemType item
* @return Whether array was fully written.
*/
boolean writeArray(Object arr, long off, int len, int bytes) {
- assert arr != null;
- assert arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
+ assert arr == null || arr.getClass().isArray() && arr.getClass().getComponentType().isPrimitive();
Review Comment:
Ok, didn't notice that, thanks for the explanation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993223605
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -65,6 +66,18 @@ public class DirectByteBufferStreamImplV1 implements DirectByteBufferStream {
/** Poison object. */
private static final Object NULL = new Object();
+ /** Flag that indicates that byte buffer is not null. */
+ private static final byte BYTE_BYFFER_NOT_NULL_FLAG = 1;
Review Comment:
Byffe, The Javampire Slayer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1189: IGNITE-17874 Added List and ByteBuffer support to message serialization
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1189:
URL: https://github.com/apache/ignite-3/pull/1189#discussion_r993230392
##########
modules/network/src/main/java/org/apache/ignite/internal/network/direct/stream/DirectByteBufferStreamImplV1.java:
##########
@@ -1049,6 +1153,80 @@ public BitSet readBitSet() {
return arr != null ? BitSet.valueOf(arr) : null;
}
+ @Override
+ public ByteBuffer readByteBuffer() {
+ byte[] bytes;
+
+ switch (byteBufferState) {
+ case 0:
+ byteBufferFlag = readByte();
+
+ boolean isNull = (byteBufferFlag & BYTE_BYFFER_NOT_NULL_FLAG) == 0;
+
+ if (!lastFinished || isNull) {
+ return null;
+ }
+
+ byteBufferState++;
+
+ //noinspection fallthrough
+ case 1:
+ if ((byteBufferFlag & BYTE_BYFFER_HAS_POSITION_FLAG) == 0) {
+ byteBufferPosition = 0;
+ } else {
+ byteBufferPosition = readInt();
+ }
+
+ if (!lastFinished) {
Review Comment:
Ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org