-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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.