Skip to content
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

ByteBuffer pooling and reuse #61

Closed
lbensaad opened this issue Sep 2, 2014 · 11 comments
Closed

ByteBuffer pooling and reuse #61

lbensaad opened this issue Sep 2, 2014 · 11 comments

Comments

@lbensaad
Copy link
Contributor

lbensaad commented Sep 2, 2014

I want to use flatbuffers with a ByteBuffer pooler or reuse an existing ByteBuffer, so it will be nice if add a constructor to the FlatBufferBuilder that accepts a ByteBuffer as parameter.

@ghost
Copy link

ghost commented Sep 2, 2014

Agreed, sounds like a good feature.

@lbensaad
Copy link
Contributor Author

lbensaad commented Sep 3, 2014

Good, especially if this library is described as "Memory Efficient Serialization Library" so adding this feature should be a core feature, I also plan to use it with Netty which use pooling also.
It could be something like this:
public FlatBufferBuilder(ByteBuffer bb)
{
this.bb = bb;
this.bb.order(ByteOrder.LITTLE_ENDIAN);
space=this.bb.position();
}

@ghost
Copy link

ghost commented Sep 3, 2014

why space=this.bb.position() ? FlatBuffers writes to ByteBuffers in an absolute way, and does not rely on position() being anywhere in particular. space represents the amount of space left over at the beginning of the buffer, so should be set to the size of the backing array at the start.

@ghost
Copy link

ghost commented Sep 3, 2014

Just added such a constructor as part of the last commit.

@ghost ghost closed this as completed Sep 3, 2014
@lbensaad
Copy link
Contributor Author

lbensaad commented Sep 4, 2014

That is good, thank you,

@lbensaad
Copy link
Contributor Author

lbensaad commented Sep 4, 2014

I see that you clear the Buffer before using it, what if the buffer contains other data? But i don't know if this is a possible scenario!

@ghost
Copy link

ghost commented Sep 4, 2014

the clear() call only resets the writable area, it doesn't actually clear
the bytes. But yes, to create a FlatBuffer, you want to be using the entire
ByteBuffer, concatenating isn't supported.

@lbensaad
Copy link
Contributor Author

lbensaad commented Sep 5, 2014

I think there is another problem with direct buffers, they don't have array(), and the FlatBufferBuilder uses it a lot so it will cause a problem.
I think you should throw UnsuportedOperationException if the the provided buffer has no array (make a call to hasArray() to know that)

@ghost
Copy link

ghost commented Sep 5, 2014

ok, will add that.

@lbensaad
Copy link
Contributor Author

lbensaad commented Sep 6, 2014

  1. It could be a better solution to use bb.capacity() instead of dd.array().length; I did a Replace All in my code editor and test it and it did work, you can do more testing.
  2. You should also avoid the use of bb.array() and System.arrayCopy like in fbb.growByteBuffer() and fbb.createString. You should use bb.put(byte[]) and bb.get(byte[]) instead.
  3. i think growByteBuffer() and newByteBuffer() should be static functions.

@ghost
Copy link

ghost commented Sep 8, 2014

  1. I already did that.
  2. The reason I didn't use put and get is that there are no absolute versions of those calls available, meaning you need to explicitly set the position.. though I suppose that is no big deal.
  3. Agreed.

kakikubo pushed a commit to kakikubo/flatbuffers that referenced this issue Apr 19, 2016
…location

support LOCATION CHARACTER UI in distribution file
This issue was closed.
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

No branches or pull requests

1 participant