msgpack-core v07 API #82

xerial · 2014-05-09T23:22:11Z

I wrote a draft code of msgpack-core for v07, which is a low-level module for reading/writing data in the Message Pack format (https://github.com/msgpack/msgpack/blob/master/spec.md).

Please review the design of this msgpack-core API in terms of the performance, usability and class/method naming. After the review I will add some test code to complete the implementation.

The goal of msgpack-core is to provide a low-level packer and unpacker implementation that is faster than v06. So, several rich functions (e.g., Template generation, JSON support) will not be implemented in msgpack-core v07.

msgpack-core API includes the following classes:

MessageBuffer

For reading/writing primitive type values in a memory
getXXX for reading values, putXXX for writing values
This class uses sun.misc.Unsafe API to read the data in memory as fast as possible
The specification of message pack requires writing short/int/float/long/double values in big-endian. The default implementation translates little-endian values into big-endian. MessageBufferBE that does not translate the endian will be used for big-endian machine.
This API tries to remove invokeinterface call to achieve the highest performance. So MessageBuffer does not implement any interface, and also to avoid the cost of TypeProfile lookup in JVM, only one of MessageBuffer or MessageBufferBE classes is loaded at runtime (using reflection).

MessagePacker

packInt, packBoolean, ... for writing message-packed values
writePayload for writing binary/raw string data

MessageBufferOutput

Used in MessagePacker to output message-packed data to a stream or buffer
This is an adapter to OutputStream or WritableByteChannel

MessageUnpacker

unpackInt, unpackBoolean, ... for reading message-packed values
readPayload for reading binary/raw string data
To read message pack data, first call getMessageType. The user need to select an appropriate unpackXXX method according to the given MessageType (enum).

MesasgeBufferInput

Used in MessageUnpacker to retrieve a sequence of MessageBuffer that contains message packed data
This is an adapter to InputStream or ReadableByteChannel

MesasgePackException

The base class of all message-pack exceptions.
MessageTypeException for type mismatch errors
MessageFormatException for invalid inputs

MessageFormat

Enum definition that covers all of the format types in the specification

ValueType

ValueType represents a group of MessageFormats. For example ValueType.INTEGER is a group of MessageFormat.INT8, INT16, .. UINT8, etc.

MessagePack

Provides utility methods for creating packer and unpacker.

frsyuki · 2014-05-10T00:22:39Z

msgpack-core/src/main/java/org/msgpack/core/MessageIntegerOverflowException.java

+ * using a smaller types. For example, calling MessageUnpacker.unpackInt() for an integer value
+ * that is larger than Integer.MAX_VALUE will cause this exception.
+ */
+public class MessageIntegerOverflowException extends MessageTypeException {


👍 Consistent name with MessageStringCodingException

oza · 2014-05-10T15:04:53Z

Should we add license information to headers?

//
// 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.
//

oza · 2014-05-10T15:27:05Z

msgpack-core/src/main/java/org/msgpack/core/MessageBuffer.java

-        this.writeBufferChunkSize = builder.getWriteBufferChunkSize();
-        this.readReferenceThreshold = builder.getReadReferenceThreshold();
-    }
+    private AtomicInteger referenceCounter;


This variable looks unused variable. Do you have any plan to support GC-like feature by using reference counter in this class? If the answer is positive, should we write it down as TODO comment?

I'm considering managing allocated MssageBuffer(s) via GC or releasing explicitly, but this feature is not yet implemented. right. It should be marked as TODO.

oza · 2014-05-10T15:46:22Z

Can we add javax.annotation.Nullable annotation to user-facing APIs? It's useful for users to understand whether arguments and return values can be null or not.

oza · 2014-05-10T15:48:48Z

msgpack-core/src/main/java/org/msgpack/core/MessageBufferOutput.java

+    private final WritableByteChannel channel;
+
+    MessageBufferOutputChannel(WritableByteChannel channel) {
+        this.channel = channel;


Same as MessageBufferOutputStream's constructor(need null check?).

…at.valueOf().getValueType() to translate byte codes. #82 (diff)

komamitsu · 2014-05-13T03:53:41Z

msgpack-core/src/main/java/org/msgpack/core/MessageUnpacker.java

+            ArrayList<MessageBuffer> bufferList = new ArrayList<MessageBuffer>();
+            while(bufferTotal < readSize) {
+                MessageBuffer next = in.next();
+                if(next == null)


Why don't you set reachedEOF to true?

Really good point. I actually modified this in another PR: https://github.com/msgpack/msgpack-java/pull/86/files#diff-6a54557ce799041aa918fa8fa48bc78dR134

komamitsu · 2014-05-13T07:03:14Z

Other than those minor comments, it looks good. GJ!

xerial · 2014-05-15T05:07:23Z

I extracted several remaining issues discussed here as tickets:
https://github.com/msgpack/msgpack-java/issues?milestone=1&state=open

I'm going to merge these changes into v07 branch.

oza · 2014-05-17T12:01:08Z

GJ, @xerial!

frsyuki added this to the v07 milestone May 9, 2014

frsyuki reviewed May 10, 2014
View reviewed changes

oza reviewed May 10, 2014
View reviewed changes

xerial added a commit that referenced this pull request May 12, 2014

Removed the similar table lookup code. Now ValueType uses MessageForm…

eb42579

…at.valueOf().getValueType() to translate byte codes. #82 (diff)

This was referenced May 12, 2014

msgpack-core v07: Using annotations and preconditions #83

Merged

Move ExtendedTypeHeader to the upper level #84

Merged

xerial added a commit that referenced this pull request May 12, 2014

Removed EOF types according to this discussion: #82 (diff)

ba99078

xerial added a commit that referenced this pull request May 12, 2014

Avoid memory copy when writing payloads in MessagePacker. #82 (diff)

d50670b

xerial mentioned this pull request May 12, 2014

Avoid memory copy in MessagePacker.writePayload #87

Merged

komamitsu reviewed May 13, 2014
View reviewed changes

Merge branch 'v07-buffer-package' into v07-draft

1620ca0

This was referenced May 15, 2014

MessageBufferInput.next() #91

Closed

Supporting event-driven I/O in unpackXXX #92

Open

Preparing extra buffer for packString and unpackString #93

Closed

Using skip length table in MessageUnpacker #94

Open

xerial merged commit 1620ca0 into v07 May 15, 2014

xerial added a commit that referenced this pull request May 15, 2014

#82 Merge v07-draft into v07

0091886

xerial deleted the v07-draft branch May 22, 2014 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

msgpack-core v07 API #82

msgpack-core v07 API #82

Uh oh!

xerial commented May 9, 2014

Uh oh!

frsyuki May 10, 2014

Uh oh!

oza commented May 10, 2014

Uh oh!

oza May 10, 2014

Uh oh!

xerial May 10, 2014

Uh oh!

oza commented May 10, 2014

Uh oh!

oza May 10, 2014

Uh oh!

komamitsu May 13, 2014

Uh oh!

xerial May 13, 2014

Uh oh!

komamitsu commented May 13, 2014

Uh oh!

xerial commented May 15, 2014

Uh oh!

oza commented May 17, 2014

Uh oh!

Uh oh!

msgpack-core v07 API #82

msgpack-core v07 API #82

Uh oh!

Conversation

xerial commented May 9, 2014

MessageBuffer

MessagePacker

MessageBufferOutput

MessageUnpacker

MesasgeBufferInput

MesasgePackException

MessageFormat

ValueType

MessagePack

Uh oh!

frsyuki May 10, 2014

Choose a reason for hiding this comment

Uh oh!

oza commented May 10, 2014

Uh oh!

oza May 10, 2014

Choose a reason for hiding this comment

Uh oh!

xerial May 10, 2014

Choose a reason for hiding this comment

Uh oh!

oza commented May 10, 2014

Uh oh!

oza May 10, 2014

Choose a reason for hiding this comment

Uh oh!

komamitsu May 13, 2014

Choose a reason for hiding this comment

Uh oh!

xerial May 13, 2014

Choose a reason for hiding this comment

Uh oh!

komamitsu commented May 13, 2014

Uh oh!

xerial commented May 15, 2014

Uh oh!

oza commented May 17, 2014

Uh oh!

Uh oh!