Add utility methods to pack/unpack to/from byte arrays efficiently #310
Conversation
There was a problem hiding this comment.
Storing default factory class as a mutable static variable doesn't seem a good idea. For instance, if a dependent library uses MessagePack and internally sets a customized factory, it'll unexpectedly affect the application that uses MessagePack.
BTW, I'm wondering why we don't directly use MessagePackFactory without MessagePack.
There was a problem hiding this comment.
Good point. This setter for the default factory should be removed.
Fix MessageFormat.Code#isFixedMap
There was a problem hiding this comment.
Do we need these configuration setters in MessageUnpacker? I think these method should be moved to the factory (or config) side.
…ker/Unpacker, which do not need to depend on MessageFormat enum
|
@komamitsu I have finished the code review and applied your comments and made minor modifications. Let me know if you finish the review. I'll merge this PR (by resolving conflicts with the v07-develop) |
There was a problem hiding this comment.
We'd better create a constant variable and replace 8192 with it.
There was a problem hiding this comment.
It should be if instead of while here?
There was a problem hiding this comment.
No. MessageBufferInput might return 0-size buffer.
There was a problem hiding this comment.
Nil is 1-byte format and this is already checked with mf.getValueType(). readByte() is used for forwarding the buffer position.
There was a problem hiding this comment.
Ah, mf.getValueType() == NIL means the value is nil. Okay.
How about changing https://github.com/msgpack/msgpack-java/pull/310/files#diff-6a54557ce799041aa918fa8fa48bc78dR537 as well?
There was a problem hiding this comment.
How about buffer or something instead of one character variable name?
|
LGTM |
|
@xerial @komamitsu Thank you! |
|
@xerial about MessagePack.Code vs MessageFormat.Code: I wanted users to not aware of formats because most of users care about semantics (API) but don't want to care about syntax (formats). I first moved MessagePack.Code to MessageFormat.Code because MessagePack is the entry point (which is used by most of users) and users will need to care about methods/classes in it, and that's not expected to have Code there. How do you think about this? |
|
@xerial about MessagePackFactory: I preferred to have a single FactoryClass to make it easy to configure MessagePack as a single library (maybe using a configuration file). I expected following example use case: MessagePackFactory factory = MyIoManagerConfigurator.loadMessagePackConfiguration("myio.config");
MessagePacker packer = factory.newPacker(...);
MessageUnpacker unpacker = factory.newUnpacker(...);But as you mention, having 2 separated classes contributes to shorten parameter names. So, it was just a preference. |
|
@xerial about MessagePackFactory.inputBufferSize and MessagePackFactory.outputBufferSize. Sorry, my code had a bug where I think buffer size should be configurable because it's a trade-off of performance and memory consuption. |
|
@frsyuki Defining Code inside MessagePackFormat introduces unnecessary dependencies between MessagePacker/Unpacker to MessageFormat. So extracting Code class outside MessagePack might be better. |
This pull-request assumes #304 and #305, and provides utility methods to use msgpack-java more efficiently. Intention of this pull-request is that users use msgpack-java using intuitive APIs and those code become automatically the fastest code.
MessagePack.newBufferPacker()method to serialize objects into a byte array (or list of byte arrays). This is easier than ByteArrayOutputStream and a lot more efficient. Internally, this implements the optimization mentioned at Add support for external buffer pools to packer #305.MessagePack.newDefaultUnpacker(byte[], int, int)