FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

expose method to create parser from direct memory by arnaudroger · Pull Request #435 · msgpack/msgpack-java · GitHub

expose method to create parser from direct memory - #435

Open
arnaudroger wants to merge 1 commit into
msgpack:mainfrom
arnaudroger:dm2
Open

expose method to create parser from direct memory#435
arnaudroger wants to merge 1 commit into
msgpack:mainfrom
arnaudroger:dm2

Conversation

Copy link
Copy Markdown

Useful to parse message coming in from netty for example without creating a lot of garbage.
Also expose the method to create a parser from a sub array.

xerial requested a review from komamitsu October 21, 2017 01:44

xerial commented Oct 21, 2017

Copy link
Copy Markdown
Member

@komamitsu Could you review this change? This seems a good improvement.

komamitsu left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@arnaudroger Thanks for the contribution! It seems a cool feature.

But I have some questions. Can I ask you to take a look at them?

* @param address the address
* @param offset the offset
* @param length the length
* @return a new ByteBufferInput on the specified address

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This static method returns ByteBuffer not ByteBufferInput

*/
public static ByteBuffer directBuffer(long address, int offset, int length)
{
return DirectBufferAccess.newByteBuffer(address, offset, length, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It doesn't seem to me that this method is related to ByteBufferInput.

@xerial Do you think this is the right place to put the method in?

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can make DirectBufferAccess a public class instead of adding this method.

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually this location is a bit weird.

IOContext ioContext = _createContext(data, false);
return _createParser(data, offset, length, ioContext);
}

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As you might notice since this method doesn't have @Override annotation, Jackson doesn't support this kind of API, I think.

How do you use this method via Jackson's API? It would be great, if you add some unit tests to make it clear 😺

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I use the MessagePackFactory directly, currently using reflection.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL