BufferUtils.copyJNI as public method?

[dustContributor]

Hi! I was trying to make use of BufferUtils but I'm having difficulties.

Thing is, most methods call copyJNI, but also do additional checks (say, instanceof) or do operations in the buffers (flip buffer, set position, etc) that I'd rather do on my own instead of having to save the state of the buffer, then use BufferUtils, then restore as it was.

This stems from my usage of LibGDX which I understand it won't be the same for everyone. I already have my own buffer utils that I'm currently using, and I'm filling the gaps with LibGDX. Having copyJNI wrapped up in other "convenience" methods makes it harder to integrate into the code I already have (and I'd really like to try it out since I spend lots of time putting data in buffers before uploading them to UBOs).

What do you think?

[xoppa]

This is a bit vague, consider being more specific. Which method (signature) are you referring to? Are you referring to the positionInBytes method when you say instanceof? Perhaps send a pull request to propose a fix and/or provide the code needed to reproduce the issue you're reporting.

[dustContributor]

Yes, thats one of the cases. Just look at the first method in BufferUtils

https://github.com/libgdx/libgdx/blob/master/gdx/src/com/badlogic/gdx/utils/BufferUtils.java

It resets the position of the buffer, and does instanceof checks to set the limit. All the other methods also try to set the limit and/or position of the buffers.

What I'm saying is whats on the title: Make 'copyJNI' methods public, the methods from line 576 to line 608. For the reasons I described in the OP (hard to integrate the methods that wrap 'copyJNI' since they have other side effects beyond just copying the data).

[MobiDevelop]

I don't think we have exposed any of our jni methods publicly throughout
libgdx because that exposes implementation details that will differ based
on platform. Not sure if we want to break this tradition.

[dustContributor]

I understand, but BufferUtils implementation differs from platform to platform? Then again, its nothing you wouldn't trip over if you called the public methods anyway.

[MobiDevelop]

BufferUtils is emulated in GWT, so yes it differs.
On Sep 14, 2014 7:18 PM, "dustContributor" notifications@github.com wrote:

I understand, but BufferUtils implementation differs from platform to
platform? Then again, its nothing you wouldn't trip over if you called the
public methods anyway.

—
Reply to this email directly or view it on GitHub
#2337 (comment).

[dustContributor]

So what you're saying is that copyJNI doesn't even exists in GWT?

[MobiDevelop]

Correct, it does not exist because there is no jni.

[dustContributor]

Yeah, just saw that, I found the GWT implementation.

And what about a version of this method (and int, double, etc variants):

public static void copy (float[] src, int srcOffset, int numElements, Buffer dst) {
    copyJni(src, srcOffset << 2, dst, positionInBytes(dst), numElements << 2);
}

But with dstOffsetBytes as parameter like:

public static void copy(float[] src, int srcOffset, int numElements, Buffer dst, int dstOffsetBytes ){
    copyJni(src, srcOffset << 2, dst, dstOffsetBytes, numElements << 2);
}

[xoppa]

So, the issue is that you don't want to use Buffer#position(int) to set the position in the buffer where you want to copy to (which is how it is intended) and instead want to manually specify that position as argument of the method? Can you explain why using Buffer#position is an issue?

Please note that if we would add the method signature as you suggested, that will probably cause confusion, because e.g. srcOffset and numElements is specified in number of floats (in your example) and dstOffsetBytes in bytes.

[badlogic]

I'm not in favor of this change. As Xoppa said, i'd like to know what the
problem is with setting yhe position.
On Sep 15, 2014 2:59 PM, "Xoppa" notifications@github.com wrote:

So, the issue is that you don't want to use Buffer#position(int)
http://docs.oracle.com/javase/7/docs/api/java/nio/Buffer.html#position(int)
to set the position in the buffer where you want to copy to (which is how
it is intended) and instead want to manually specify that position as
argument of the method? Can you explain why using Buffer#position is an
issue?

Please note that if we would add the method signature as you suggested,
that will probably cause confusion, because e.g. srcOffset and numElements
is specified in number of floats (in your example) and dstOffsetBytes in
bytes.

—
Reply to this email directly or view it on GitHub
#2337 (comment).

[dustContributor]

So, the issue is that you don't want to use Buffer#position(int) to set the position in the buffer where you want to copy to (which is how it is intended) and instead want to manually specify that position as argument of the method? Can you explain why using Buffer#position is an issue?

All right. Forget about the position/limit thing. Focus on the BufferUtils.copy method I quoted on my last comment.

That call computes the srcOffset in bytes directly, which is great, computes the numElements in btyes directly, which is also great, but does that ugly BufferUtils#positionInBytes(int) call inside that can be avoided IF the user knows what kind of buffer he is passing to the method.

What I want is a new method that lets me pass the offsets by myself.

For example:

public void updateBuffer ( RenderInstance item ) {
FloatBuffer view = tmpBuffer.view;
Spatial sp = item.spatial;
view.put( sp.mvpTransform.array );
view.put( sp.mvTransform.array );
}

Used like:

for ( RenderInstance item: renderInstances )
    updateBuffer(item);

That's updating a buffer before uploading to an UBO. Transforms are sequentially stored in the buffer in the form of { mvp1, mv1, mvp2, mv2, mvp3, mv3, ... mvpx, mvx }.

Closest replacement with BufferUtils would be:

FloatBuffer view = tmpBuffer.view;
Spatial sp = item.spatial;
BufferUtils.copy( sp.mvpTransform.array, 0, 16, view);
view.position( view.position() + 16 );
BufferUtils.copy( sp.mvTransform.array, 0, 16, view);
view.position( view.position() + 16 );

Inside those BufferUtils.copy calls there is that BufferUtils#positionInBytes(int) call which goes a long if/else chain to get the actual offset.

Instead I could just do:

FloatBuffer view = tmpBuffer.view;
Spatial sp = item.spatial;
int offsetInBytes = view.position() << 2;
BufferUtils.copy( sp.mvpTransform.array, 0, 16, view, offsetInBytes);
view.position( view.position() + 16 );
offsetInBytes += 64; // 16 floats * 4 bytes each.
BufferUtils.copy( sp.mvTransform.array, 0, 16, view, offsetInBytes);
view.position( view.position() + 16 );

No more needless branching because I already know the actual offsets.

BufferUtils.copy implementation would be just like the one I commented above:

public static void copy(float[] src, int srcOffset, int numElements, Buffer dst, int dstOffsetBytes ){
copyJni(src, srcOffset << 2, dst, dstOffsetBytes, numElements << 2);
}

No need for BufferUtils#positionInBytes(int) anymore.

Please note that if we would add the method signature as you suggested, that will probably cause confusion, because e.g. srcOffset and numElements is specified in number of floats (in your example) and dstOffsetBytes in bytes.

That's why its called dstOffsetBytes. You can't possibly know what Buffer actual class is, unless you do a long if/else chain of instanceof as most BufferUtils methods do. So the next best thing is directly specifying the offset in bytes. The user of the method probably will know what kind of buffer is passing to the method, so he can compute the offset in the spot without branching needlessly.

In any case, I've already found that BufferUtils.copy works terribly slow for my use case. So I might just put everything in a float[] and do a single put(float[]) call on the buffer, or see if in that case BufferUtils.copy works better (then the positionInBytes won't be a problem since it will be called few times).

[xoppa]

So, to summarize: the issue you're reporting is that the BufferUtils.positionInBytes(int) method is called which does branching that can be avoided when the caller knows the type of buffer. Is that correct?

[dustContributor]

Yes. Although if you're thinking of overloads with IntBuffer, FloatBuffer, and so on as a possible solution. I don't think its a good idea. It limits what the caller can put in a buffer.

Say that I don't have a FloatBuffer view of my ByteBuffer and I still want to put a float array on it. If the only way was through an overloaded BufferUtils.copy that receives a FloatBuffer, the user would have to create a FloatBuffer view of the buffer just to make the call.

That's why I'm advocating just adding a BufferUtils.copy that allows you to pass the offset in bytes. It's the simplest way and allows you the freedom of passing any type of buffer you want.

[MobiDevelop]

The proposed method would make no sense for GWT as the dstOffsetInBytes is an implementation detail specific to the jni implementation. Granted, you could just ignore this method exists from a GWT standpoint, however, it does leak implementation details into the public api and I'd think that we'd want to avoid that when possible.

[xoppa]

The branch can be avoided when the caller knows the type of buffer. But why would the branch (besides from being not necessary in some scenario's) be an issue? I have a feeling (didn't actually profile) that the overhead of the jni call itself is probably way more significant (on dalvik/jvm) than the branch. The proposed change might complicate or bloat the api at close to no gain. @dustContributor, did you actually profile this and found that it is indeed the branch that is the issue?

[dustContributor]

The proposed method would make no sense for GWT [...] it does leak implementation details into the public api and I'd think that we'd want to avoid that when possible.

You're dealing with native buffers in a multi platform library, what else you could expect but to deal with the details of the platform that you're running on? I doubt it will shock any Android programmer looking in BufferUtils that buffers deal with bytes, at least I hope it doesn't.

For example, if you pass a non-native buffer to the methods, which they allow, everything explodes. How's that not an implementation detail? Instead of checking in every single method if the buffer is an instance of DirectBuffer, you simply trust the user that they'll only pass direct buffers.

A method with dstOffsetInBytes would work essentially under the same assumptions that the rest of the methods build upon. Quoting the javadocs:

This is an expert method, use at your own risk.

The proposed change might complicate or bloat the api at close to no gain. @dustContributor, did you actually profile this and found that it is indeed the branch that is the issue?

As I mentioned, the copyJNI call runs horribly slow for my use case (I found that weird, I expected slower, but not that slower), not because the branching but because the overhead of copyJNI itself. And as I mentioned, since for small updates copyJNI isn't worth it, for very few big updates the time spent branching would be minimal.

It's more about avoiding some cruft if you want to build upon BufferUtils to do things. ie, don't pay for what you didn't asked for.

[xoppa]

Thanks! Yes, you should always try to avoid switching back and forth between java and native code when possible. It's better to build an array fully in java and then copy it once to a direct buffer (like you said). That's also how e.g. SpriteBatch and comparable classes work.

So I guess it is safe to say that the issue you were initially having wasn't caused by the branch and this issue can be closed. The branch might be considered cruft in some use-cases, although its overhead is not noticeable. But overall there's no issue with it.

As for the direct buffer requirement, quoting the javadocs:

The Buffer must be a direct Buffer with native byte order. No error checking is performed

Like said, my guess is that this issue can be closed now. However, if you still think there's an issue, then please feel free to comment and we'll reopen it as needed.

[dustContributor]

Fine by me. Thanks for considering it!