-
Notifications
You must be signed in to change notification settings - Fork 321
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
expose method to create parser from direct memory #435
base: main
Are you sure you want to change the base?
Conversation
@komamitsu Could you review this change? This seems a good improvement. |
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.
@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 |
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.
This static method returns ByteBuffer not ByteBufferInput
*/ | ||
public static ByteBuffer directBuffer(long address, int offset, int length) | ||
{ | ||
return DirectBufferAccess.newByteBuffer(address, offset, length, null); |
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.
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?
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.
We can make DirectBufferAccess a public class instead of adding this method.
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.
Actually this location is a bit weird.
return _createParser(data, offset, length, ioContext); | ||
} | ||
|
||
public JsonParser createParser(long memoryAddress, int offset, int length) |
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.
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 😺
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.
I use the MessagePackFactory directly, currently using reflection.
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.