[FIXED] Observations regarding PointerBuffer

[kappa]

Riven made some observations on the IRC channel about the PointerBuffer class, thought i'd paste it here so that it doesn't get overlooked, don't know enough about it to comment but someone should have a quick look just to make sure its an issue that needs fixing.

[22:04:43] <_Riven> regarding PointerBuffer...
[22:05:15] <_Riven> why isn't the source buffer not checked for alignment?
[22:05:41] <_Riven> line 82
[22:05:48] <MatthiasM> good point
[22:06:37] <_Riven> asIntBuffer / asLongBuffer doesn't do that for you
[22:09:28] <_Riven> while you're at it, check that source.remaining() is a multiple of either 4 or 8
[22:09:40] <_Riven> for good measure, because it's an indication of a bug
[22:10:27] <_Riven> (passing in buffers as if it's a 32 bit system)
[22:10:49] <_Riven> fail early, etc

[spasi]

Well, these are fair points, but I don't see a bug here:

- asIntBuffer / asLongBuffer will return the proper subclass, depending on byte order, memory offset and whether the hardware supports unaligned memory access or not. In the worst case you get suboptimal performance (only possible on the ARM VM basically).

- asIntBuffer / asLongBuffer will also clip any excess bytes at the end of the source buffer.

True, both scenarios might not be the developer's intent, but at the same time I can't say for sure there won't be use cases where adding these checks will limit some developer out there. Also, the source buffer might be a sliced one, simply checking .position() won't be enough, I'd need to expose GetDirectBufferAddress somewhere and use that to properly check for memory alignment. Not a big deal, but I don't see a serious reason to do this.

Could Riven or anyone else let me know if there's a legit scenario in which PointerBuffer will fail without alignment checks?

[Riven]

@kappa: Thanks for the 'repost'
@Spasi: Thanks for your reply.

It's not so much a bug, but I'm a huge advocate for fail-fast behaviour when something smells fishy.

Just look at how enhanced-for in throws a ConcurrentModificationException when you try to modify the collection you're iterating over. It doesn't mean the developer did something wrong, it means that the developer did something that will probably result in unexpected behavior.

"Garbage in, garbage out" (and the resulting silent failures) is IMHO among the worst ideas ever. In Java we should adhere to "Garbage in, raise exception" to prevent problems to escalate.

Say, when you're on a 64 bit system and the buffer has a number of remaining bytes that is a multiple of exactly 4, you can be fairly sure the developer made a mistake, which is likely to cause a nasty access violation later on (we're dealing with passing pointers here). On both 32 and 64 bit systems, the developer must manually handle the width of the pointer, which usually ends in disaster with the 'works on my machine' mentality.
It's the same for any other remaining amount of bytes, not being a multiple of 8... but 4 is especially worrying as it's almost right and hence very likely to be very wrong. I consider the clipping of remaining bytes to be a silent failure, because there was data there, and we decide to ignore it, without telling the developer.

I don't think 'suboptimal performance' is an innocent issue either, when you're dealing with OpenCL, which is all about cranking the most out of the available hardware. Unaligned memory access on non-x86/x64 CPUs is at least an order of magnitude slower, AFAIK.

Regarding the need for GetDirectBufferAddress, that's exactly what I meant.

[spasi]

OK, committed:

Code:

final int alignment = is64Bit ? 8 : 4;
if ( (BufferUtils.getBufferAddress(source) + source.position()) % alignment != 0 || source.remaining() % alignment != 0 )
throw new IllegalArgumentException("The source buffer is not aligned to " + alignment + " bytes.");

Good enough?

[Riven]

Yup