Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions msgpack-core/src/main/java/org/msgpack/core/MessagePack.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.msgpack.core;

import org.msgpack.core.buffer.ArrayBufferInput;
import org.msgpack.core.buffer.ByteBufferInput;
import org.msgpack.core.buffer.ChannelBufferInput;
import org.msgpack.core.buffer.ChannelBufferOutput;
import org.msgpack.core.buffer.InputStreamBufferInput;
Expand All @@ -25,6 +26,7 @@

import java.io.InputStream;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
import java.nio.channels.WritableByteChannel;
import java.nio.charset.Charset;
Expand Down Expand Up @@ -236,6 +238,17 @@ public static MessageUnpacker newDefaultUnpacker(byte[] contents, int offset, in
return DEFAULT_UNPACKER_CONFIG.newUnpacker(contents, offset, length);
}

/**
* Create an unpacker that reads the data from a given ByteBuffer
*
* @param contents
* @return
*/
public static MessageUnpacker newDefaultUnpacker(ByteBuffer contents)
{
return DEFAULT_UNPACKER_CONFIG.newUnpacker(contents);
}

/**
* MessagePacker configuration.
*/
Expand Down Expand Up @@ -524,6 +537,17 @@ public MessageUnpacker newUnpacker(byte[] contents, int offset, int length)
return newUnpacker(new ArrayBufferInput(contents, offset, length));
}

/**
* Create an unpacker that reads the data from a given ByteBuffer
*
* @param contents
* @return
*/
public MessageUnpacker newUnpacker(ByteBuffer contents)
{
return newUnpacker(new ByteBufferInput(contents));
}

/**
* Allow unpackBinaryHeader to read str format family (default: true)
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,7 @@ else if (s.length() < (1 << 8)) {
throw new IllegalArgumentException("Unexpected UTF-8 encoder state");
}
// move 1 byte backward to expand 3-byte header region to 3 bytes
buffer.putBytes(position + 3,
buffer.array(), buffer.arrayOffset() + position + 2, written);
buffer.putMessageBuffer(position + 3, buffer, position + 2, written);
// write 3-byte header
buffer.putByte(position++, STR16);
buffer.putShort(position, (short) written);
Expand Down Expand Up @@ -560,8 +559,7 @@ else if (s.length() < (1 << 16)) {
throw new IllegalArgumentException("Unexpected UTF-8 encoder state");
}
// move 2 bytes backward to expand 3-byte header region to 5 bytes
buffer.putBytes(position + 5,
buffer.array(), buffer.arrayOffset() + position + 3, written);
buffer.putMessageBuffer(position + 5, buffer, position + 3, written);
// write 3-byte header header
buffer.putByte(position++, STR32);
buffer.putInt(position, written);
Expand Down
18 changes: 5 additions & 13 deletions msgpack-core/src/main/java/org/msgpack/core/MessageUnpacker.java
Original file line number Diff line number Diff line change
Expand Up @@ -213,14 +213,9 @@ private MessageBuffer prepareNumberBuffer(int readLength)
// fill the temporary buffer from the current data fragment and
// next fragment(s).

// TODO buffer.array() doesn't work if MessageBuffer is allocated by
// newDirectBuffer. dd copy method to MessageBuffer to solve this issue.

int off = 0;
if (remaining > 0) {
numberBuffer.putBytes(0,
buffer.array(), buffer.arrayOffset() + position,
remaining);
numberBuffer.putMessageBuffer(0, buffer, position, remaining);
readLength -= remaining;
off += remaining;
}
Expand All @@ -229,16 +224,12 @@ private MessageBuffer prepareNumberBuffer(int readLength)
nextBuffer();
int nextSize = buffer.size();
if (nextSize >= readLength) {
numberBuffer.putBytes(off,
buffer.array(), buffer.arrayOffset(),
readLength);
numberBuffer.putMessageBuffer(off, buffer, 0, readLength);
position = readLength;
break;
}
else {
numberBuffer.putBytes(off,
buffer.array(), buffer.arrayOffset(),
nextSize);
numberBuffer.putMessageBuffer(off, buffer, 0, nextSize);
readLength -= nextSize;
off += nextSize;
}
Expand Down Expand Up @@ -1041,7 +1032,8 @@ private void handleCoderError(CoderResult cr)
private String decodeStringFastPath(int length)
{
if (actionOnMalformedString == CodingErrorAction.REPLACE &&
actionOnUnmappableString == CodingErrorAction.REPLACE) {
actionOnUnmappableString == CodingErrorAction.REPLACE &&
buffer.hasArray()) {
String s = new String(buffer.array(), buffer.arrayOffset() + position, length, MessagePack.UTF8);
position += length;
return s;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//
// MessagePack for Java
//
// 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 org.msgpack.core.buffer;

import java.io.IOException;
import java.nio.ByteBuffer;

import static org.msgpack.core.Preconditions.checkNotNull;

/**
* {@link MessageBufferInput} adapter for {@link java.nio.ByteBuffer}
*/
public class ByteBufferInput
implements MessageBufferInput
{
private ByteBuffer input;
private boolean isRead = false;

public ByteBufferInput(ByteBuffer input)
{
this.input = checkNotNull(input, "input ByteBuffer is null");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is better to call slice() as following to avoid this unexpected scenario:

  • An user creates a new ByteBuffer and fill some data there
  • The user creates a new ByteBufferInput with the ByteBuffer
  • the user changes position of the ByteBuffer! That might be by another thread.
  • ByteBufferInput is affected the change of position, unexpectedly
        this.input = checkNotNull(input, "input ByteBuffer is null").slice();

}

/**
* Reset buffer. This method doesn't close the old resource.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method doesn't close the old resource. this text seems unnecessary

*
* @param input new buffer
* @return the old resource
*/
public ByteBuffer reset(ByteBuffer input)
{
ByteBuffer old = this.input;
this.input = input;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same with above:

        this.input = checkNotNull(input, "input ByteBuffer is null").slice();

isRead = false;
return old;
}

@Override
public MessageBuffer next()
throws IOException
{
if (isRead) {
return null;
}

isRead = true;
return MessageBuffer.wrap(input);
}

@Override
public void close()
throws IOException
{
// Nothing to do
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public class MessageBuffer
* Reference to MessageBuffer Constructors
*/
private static final Constructor<?> mbArrConstructor;
private static final Constructor<?> mbBBConstructor;

/**
* The offset from the object memory header to its byte array data
Expand Down Expand Up @@ -145,6 +146,11 @@ public class MessageBuffer
Constructor<?> mbArrCstr = bufferCls.getDeclaredConstructor(byte[].class, int.class, int.class);
mbArrCstr.setAccessible(true);
mbArrConstructor = mbArrCstr;

// MessageBufferX(ByteBuffer) constructor
Constructor<?> mbBBCstr = bufferCls.getDeclaredConstructor(ByteBuffer.class);
mbBBCstr.setAccessible(true);
mbBBConstructor = mbBBCstr;
}
catch (Exception e) {
e.printStackTrace(System.err);
Expand All @@ -170,6 +176,12 @@ public class MessageBuffer
*/
protected final int size;

/**
* Reference is used to hold a reference to an object that holds the underlying memory so that it cannot be
* released by the garbage collector.
*/
protected final ByteBuffer reference;

public static MessageBuffer allocate(int length)
{
return wrap(new byte[length]);
Expand All @@ -185,6 +197,11 @@ public static MessageBuffer wrap(byte[] array, int offset, int length)
return newMessageBuffer(array, offset, length);
}

public static MessageBuffer wrap(ByteBuffer bb)
{
return newMessageBuffer(bb).slice(bb.position(), bb.remaining());
}

/**
* Creates a new MessageBuffer instance backed by a java heap array
*
Expand All @@ -202,11 +219,32 @@ private static MessageBuffer newMessageBuffer(byte[] arr, int off, int len)
}
}

/**
* Creates a new MessageBuffer instance backed by ByteBuffer
*
* @param bb
* @return
*/
private static MessageBuffer newMessageBuffer(ByteBuffer bb)
{
checkNotNull(bb);
try {
// We need to use reflection to create MessageBuffer instances in order to prevent TypeProfile generation for getInt method. TypeProfile will be
// generated to resolve one of the method references when two or more classes overrides the method.
return (MessageBuffer) mbBBConstructor.newInstance(bb);
} catch (Exception e) {
throw new RuntimeException(e);
}
}

public static void releaseBuffer(MessageBuffer buffer)
{
if (isUniversalBuffer || buffer.base instanceof byte[]) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@xerial I think that type of MessageBuffer.buffer field can be byte[] instead of Object. no? If yes, this buffer.base instanceof byte[] can be buffer.base != null.

@xerial xerial Jul 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@frsyuki If we never use this class for reading float[], long[], etc. Using byte[] is ok. If we need to read byte arrays more than 2GB, leaving this to Object is good since we can put long[] here so that we can read 8 (long byte size) * 2GB (2^31) = 16GB data.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see...but I think it's too early to think about it. Exception handling becomes more complicated with consideration of long[] there.

@xerial xerial Jul 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2GB is not so huge, so we should think about it if we are going to support huge message pack based DataFrame in memory.

The index of getXXX(index) is using int, so we already have a technical limit of the accessible buffer range, but this should not limit the underlying buffer size; if we use long[] we can create a MessageBuffer slice (2GB range) on top of the 16GB memory buffer.

// We have nothing to do. Wait until the garbage-collector collects this array object
}
else if (DirectBufferAccess.isDirectByteBufferInstance(buffer.base)) {
DirectBufferAccess.clean(buffer.base);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@miniway shouldn't this be buffer.reference because base is null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch! it should be the reference

}
else {
// Maybe cannot reach here
unsafe.freeMemory(buffer.address);
Expand All @@ -225,13 +263,43 @@ public static void releaseBuffer(MessageBuffer buffer)
this.base = arr;
this.address = ARRAY_BYTE_BASE_OFFSET + offset;
this.size = length;
this.reference = null;
}

/**
* Create a MessageBuffer instance from a given ByteBuffer instance
*
* @param bb
*/
MessageBuffer(ByteBuffer bb)
{
if (bb.isDirect()) {
if (isUniversalBuffer) {
throw new IllegalStateException("Cannot create MessageBuffer from DirectBuffer");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's include "on this platform" in the message so that people don't think it's not MessagePack's fault...

+                throw new IllegalStateException("Cannot create MessageBuffer from DirectBuffer on this platform");

}
// Direct buffer or off-heap memory
this.base = null;
this.address = DirectBufferAccess.getAddress(bb);
this.size = bb.capacity();
this.reference = bb;
}
else if (bb.hasArray()) {
this.base = bb.array();
this.address = ARRAY_BYTE_BASE_OFFSET;
this.size = bb.array().length;
this.reference = null;
}
else {
throw new IllegalArgumentException("Only the array-backed ByteBuffer or DirectBuffer are supported");
}
}

protected MessageBuffer(Object base, long address, int length)
{
this.base = base;
this.address = address;
this.size = length;
this.reference = null;
}

/**
Expand Down Expand Up @@ -393,6 +461,11 @@ else if (src.hasArray()) {
}
}

public void putMessageBuffer(int index, MessageBuffer src, int srcOffset, int len)
{
unsafe.copyMemory(src.base, src.address + srcOffset, base, address + index, len);
}

/**
* Create a ByteBuffer view of the range [index, index+length) of this memory
*
Expand All @@ -402,7 +475,13 @@ else if (src.hasArray()) {
*/
public ByteBuffer sliceAsByteBuffer(int index, int length)
{
return ByteBuffer.wrap((byte[]) base, (int) ((address - ARRAY_BYTE_BASE_OFFSET) + index), length);
if (hasArray()) {
return ByteBuffer.wrap((byte[]) base, (int) ((address - ARRAY_BYTE_BASE_OFFSET) + index), length);
}
else {
assert (!isUniversalBuffer);
return DirectBufferAccess.newByteBuffer(address, index, length, reference);
}
}

/**
Expand All @@ -415,6 +494,11 @@ public ByteBuffer sliceAsByteBuffer()
return sliceAsByteBuffer(0, size());
}

public boolean hasArray()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

{
return base instanceof byte[];

@frsyuki frsyuki Jul 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how about using base != null instead that seems faster (and safe enough)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer base != null also.

}

/**
* Get a copy of this buffer
*
Expand Down
Loading