-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Optimize writing numeric values. #1635
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
Changes from 11 commits
572efd9
c1d73ac
0e90c12
5a19a70
c693564
c7d6d4d
fce5610
01530aa
aac7ed8
53cbe5f
4ae828a
5983d1f
ee72dbc
28351b4
4b37c94
f9fcd60
183c081
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,6 +106,50 @@ public interface ByteBuf { | |
*/ | ||
ByteBuf put(byte b); | ||
|
||
/** | ||
* Writes the given int value into this buffer at the current position, | ||
* using the current byte order, and increments the position by 4. | ||
* | ||
* @param b the int value to be written | ||
* @return this buffer | ||
* @throws java.nio.BufferOverflowException if there are fewer than 4 bytes remaining in this buffer | ||
* @throws java.nio.ReadOnlyBufferException if this buffer is read-only | ||
*/ | ||
ByteBuf putInt(int b); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the interface includes the warning:
it is permissible to add new methods to this class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add |
||
|
||
/** | ||
* Writes the given int value into this buffer at the current position, | ||
* using the current byte order, and increments the position by 4. | ||
* | ||
* @param b the int value to be written | ||
* @return this buffer | ||
* @throws java.nio.BufferOverflowException if there are fewer than 4 bytes remaining in this buffer | ||
* @throws java.nio.ReadOnlyBufferException if this buffer is read-only | ||
*/ | ||
ByteBuf putInt(int index, int b); | ||
|
||
/** | ||
* Writes the given double value into this buffer at the current position, | ||
* using the current byte order, and increments the position by 8. | ||
* | ||
* @param b the double value to be written | ||
* @return this buffer | ||
* @throws java.nio.BufferOverflowException if there are fewer than 8 bytes remaining in this buffer | ||
* @throws java.nio.ReadOnlyBufferException if this buffer is read-only | ||
*/ | ||
ByteBuf putDouble(double b); | ||
|
||
/** | ||
* Writes the given long value into this buffer at the current position, | ||
* using the current byte order, and increments the position by 8. | ||
* | ||
* @param b the long value to be written | ||
* @return this buffer | ||
* @throws java.nio.BufferOverflowException if there are fewer than 8 bytes remaining in this buffer | ||
* @throws java.nio.ReadOnlyBufferException if this buffer is read-only | ||
*/ | ||
ByteBuf putLong(long b); | ||
|
||
/** | ||
* <p>Flips this buffer. The limit is set to the current position and then the position is set to zero. If the mark is defined then it | ||
* is discarded.</p> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,6 +97,30 @@ public ByteBuf put(final byte b) { | |
return this; | ||
} | ||
|
||
@Override | ||
public ByteBuf putInt(final int b) { | ||
buf.putInt(b); | ||
return this; | ||
} | ||
|
||
@Override | ||
public ByteBuf putInt(final int index, final int b) { | ||
buf.putInt(index, b); | ||
return this; | ||
} | ||
|
||
@Override | ||
public ByteBuf putDouble(final double b) { | ||
buf.putDouble(b); | ||
return this; | ||
} | ||
|
||
@Override | ||
public ByteBuf putLong(final long b) { | ||
buf.putLong(b); | ||
return this; | ||
} | ||
|
||
@Override | ||
public ByteBuf flip() { | ||
((Buffer) buf).flip(); | ||
|
@@ -160,8 +184,13 @@ public ByteBuf get(final byte[] bytes, final int offset, final int length) { | |
|
||
@Override | ||
public ByteBuf get(final int index, final byte[] bytes, final int offset, final int length) { | ||
for (int i = 0; i < length; i++) { | ||
bytes[offset + i] = buf.get(index + i); | ||
if (buf.hasArray()) { | ||
System.arraycopy(buf.array(), index, bytes, offset, length); | ||
} else { | ||
// Fallback to per-byte copying if no backing array is available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually Netty ByteBufs already have optimized methods with this signature - and most of the time the buffers are off-heap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify, do you mean Netty ByteBuf's I believe we’re already using that one in our
This particular method is targeting the Java NIO |
||
for (int i = 0; i < length; i++) { | ||
bytes[offset + i] = buf.get(index + i); | ||
} | ||
} | ||
return this; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
/* | ||
* Copyright 2008-present MongoDB, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package com.mongodb.internal.connection; | ||
|
||
|
||
import org.bson.ByteBuf; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.MethodSource; | ||
|
||
import java.util.stream.Stream; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
|
||
class ByteBufTest { | ||
|
||
static Stream<BufferProvider> bufferProviders() { | ||
return Stream.of(new ByteBufSpecification.NettyBufferProvider(), new SimpleBufferProvider()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("bufferProviders") | ||
void shouldPutInt(final BufferProvider provider) { | ||
ByteBuf buffer = provider.getBuffer(1024); | ||
try { | ||
buffer.putInt(42); | ||
buffer.flip(); | ||
assertEquals(42, buffer.getInt()); | ||
} finally { | ||
buffer.release(); | ||
} | ||
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("bufferProviders") | ||
void shouldPutLong(final BufferProvider provider) { | ||
ByteBuf buffer = provider.getBuffer(1024); | ||
try { | ||
buffer.putLong(42L); | ||
buffer.flip(); | ||
assertEquals(42L, buffer.getLong()); | ||
} finally { | ||
buffer.release(); | ||
} | ||
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("bufferProviders") | ||
void shouldPutDouble(final BufferProvider provider) { | ||
ByteBuf buffer = provider.getBuffer(1024); | ||
try { | ||
buffer.putDouble(42.0D); | ||
buffer.flip(); | ||
assertEquals(42.0D, buffer.getDouble()); | ||
} finally { | ||
buffer.release(); | ||
} | ||
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("bufferProviders") | ||
void shouldPutIntAtIndex(final BufferProvider provider) { | ||
rozza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ByteBuf buffer = provider.getBuffer(1024); | ||
try { | ||
buffer.putInt(0); | ||
buffer.putInt(0); | ||
buffer.putInt(0); | ||
buffer.putInt(0); | ||
buffer.put((byte) 43); | ||
buffer.put((byte) 44); | ||
buffer.putInt(0, 22); | ||
buffer.putInt(4, 23); | ||
buffer.putInt(8, 24); | ||
buffer.putInt(12, 25); | ||
buffer.flip(); | ||
|
||
assertEquals(22, buffer.getInt()); | ||
assertEquals(23, buffer.getInt()); | ||
assertEquals(24, buffer.getInt()); | ||
assertEquals(25, buffer.getInt()); | ||
assertEquals(43, buffer.get()); | ||
assertEquals(44, buffer.get()); | ||
} finally { | ||
buffer.release(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Note] As @stIncMale suggested, lets put Evolving annotation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stIncMale I will add it in a follow-up PR as we don't have this annotation in
Bson
package.