>>, UNSAFE.getObject(buffer, offsetOffset) Obviously should be Unsafe.getInt(buffer, offsetOffset)
On Thu, Oct 23, 2014 at 11:16 AM, Stanimir Simeonoff <stani...@riflexo.com> wrote: > Unsafe is available, so the fields (array, offset) can be read directly > UNSAFE.getObject(buffer, hbOffset), UNSAFE.getObject(buffer, offsetOffset). > No need for MethodHandlers. > During class init the offsets have to be resolved, pretty much like any > CAS utilizing algorithm. > > I didn't propose it as readOnlyBuffers are very, very rarely used and even > more unlikely to be used to calculate checksums. It just makes the code > ugly. > > Stanimir > > On Thu, Oct 23, 2014 at 11:05 AM, Peter Levart <peter.lev...@gmail.com> > wrote: > >> On 10/23/2014 03:52 AM, Staffan Friberg wrote: >> >>> Webrev with these last updates. Added more tests to make sure CRC32C, >>> CRC32 and Checksum default methods all are covered. >>> >>> http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.07 >>> >> >> Hi Staffan, >> >> Regarding default case: >> >> 168 } else { >> 169 byte[] b = new byte[Math.min(buffer.remaining(), 4096)]; >> 170 while (buffer.hasRemaining()) { >> 171 int length = Math.min(buffer.remaining(), b.length); >> 172 buffer.get(b, 0, length); >> 173 update(b, 0, length); >> 174 } >> 175 } >> >> >> Have you tried using get()/getInt() directly on the (ro) ByteBuffer >> instead of copying to byte[] chunks? Intuitively one would expect it >> perform faster if a redundant copy is avoided. Ah, you already told us that >> you plan to use intrinsic for CRC32C in the future, so you want to have >> "addresses" at hand. >> >> A hackish way to avoid copying in this case is to access the byte[] and >> offset using reflection. But this would have to be wrapped with >> doPrivileged() which would worsen performance for small buffers. A way to >> avoid repeated access checks is to do them at class initialization time, >> using MethodHandle(s). For example, something like: >> >> >> private static final MethodHandle bbArrayGetter; >> private static final MethodHandle bbArrayOffsetGetter; >> >> static { >> MethodHandle hbGetter; >> MethodHandle offsetGetter; >> try { >> Field hbField = ByteBuffer.class.getDeclaredField("hb"); >> Field offsetField = ByteBuffer.class. >> getDeclaredField("offset"); >> AccessController.doPrivileged(new PrivilegedAction<Void>() { >> @Override >> public Void run() { >> hbField.setAccessible(true); >> offsetField.setAccessible(true); >> return null; >> } >> }); >> hbGetter = MethodHandles.lookup().unreflectGetter(hbField); >> offsetGetter = MethodHandles.lookup(). >> unreflectGetter(offsetField); >> } catch (NoSuchFieldException | IllegalAccessException e) { >> hbGetter = null; >> offsetGetter = null; >> } >> bbArrayGetter = hbGetter; >> bbArrayOffsetGetter = offsetGetter; >> } >> >> private static byte[] getArrayOrNull(ByteBuffer bb) { >> if (bb.hasArray()) return bb.array(); >> if (bbArrayGetter != null) { >> try { >> return (byte[]) bbArrayGetter.invokeExact(bb); >> } catch (Throwable e) { >> throw new InternalError(e); >> } >> } >> return null; >> } >> >> private static int getArrayOffset(ByteBuffer bb) { >> if (bb.hasArray()) return bb.arrayOffset(); >> if (bbArrayOffsetGetter != null) { >> try { >> return (int) bbArrayOffsetGetter.invokeExact(bb); >> } catch (Throwable e) { >> throw new InternalError(e); >> } >> } >> throw new UnsupportedOperationException(); >> } >> >> >> >> Regards, Peter >> >> >>> //Staffan >>> >>> On 10/22/2014 05:37 PM, Stanimir Simeonoff wrote: >>> >>>> Hi Staffan, >>>> >>>> The readonly buffer (ByteBuffer.asReadOnlyBuffer()) don't have array() >>>> "working". >>>> You can use "int length = Math.min(buffer.remaining, b.length)" >>>> instead, same with new byte[Math.min(4096, buffer.remaining)]. Using >>>> smaller chunks will be more performance friendly than allocating/eating up >>>> a huge byte[]. >>>> If you feel like, add a test with a heap bytebuffer.asReadOnlyBuffer(). >>>> >>>> Stanimir >>>> >>>> >>>> On Thu, Oct 23, 2014 at 3:06 AM, Staffan Friberg < >>>> staffan.frib...@oracle.com <mailto:staffan.frib...@oracle.com>> wrote: >>>> >>>> Hi, >>>> >>>> I was thinking about this earlier when I started writing the patch >>>> and then I forgot about it again. I haven't been able to figure >>>> out when the code will be executed. ByteBuffer is implemented in >>>> such a way that only the JDK can extend it and as far as I can >>>> tell you can only create 3 types of ByteBuffers (Direct, Mapped >>>> and Heap), all of which will be handled by the more efficient >>>> calls above. >>>> >>>> That said just to make the code a bit safer from OOM it is >>>> probably best to update the default method and all current >>>> implementations which all use the same pattern. >>>> >>>> A reasonable solution should be the following code >>>> >>>> byte[] b = new byte[(buffer.remaining() < 4096) >>>> ? buffer.remaining() : 4096]; >>>> while (buffer.hasRemaining()) { >>>> int length = (buffer.remaining() < b.length) >>>> ? buffer.remaining() : b.length; >>>> buffer.get(b, 0, length); >>>> update(b, 0, length); >>>> } >>>> >>>> Xueming, do you have any further comment? >>>> >>>> Regards, >>>> Staffan >>>> >>>> On 10/22/2014 03:04 PM, Stanimir Simeonoff wrote: >>>> >>>>> >>>>> >>>>> On Thu, Oct 23, 2014 at 12:10 AM, Bernd Eckenfels >>>>> <e...@zusammenkunft.net <mailto:e...@zusammenkunft.net>> wrote: >>>>> >>>>> Hello, >>>>> >>>>> just a question in the default impl: >>>>> >>>>> + } else { >>>>> + byte[] b = new byte[rem]; >>>>> + buffer.get(b); >>>>> + update(b, 0, b.length); >>>>> + } >>>>> >>>>> would it be a good idea to actually put a ceiling on the size >>>>> of the >>>>> array which is processed at once? >>>>> This is an excellent catch. >>>>> Should not be too large, probably 4k or so. >>>>> >>>>> Stanimir >>>>> >>>>> >>>>> Am Tue, 21 Oct 2014 10:28:50 -0700 >>>>> schrieb Staffan Friberg <staffan.frib...@oracle.com >>>>> <mailto:staffan.frib...@oracle.com>>: >>>>> >>>>> >>>>> > Hi Peter, >>>>> > >>>>> > Thanks for the comments.. >>>>> > > >>>>> > > 217 if (Unsafe.ADDRESS_SIZE == 4) { >>>>> > > 218 // On 32 bit platforms read two >>>>> ints >>>>> > > instead of a single 64bit long >>>>> > > >>>>> > > When you're reading from byte[] using Unsafe >>>>> (updateBytes), you >>>>> > > have the option of reading 64bit values on 64bit >>>>> platforms. When >>>>> > > you're reading from DirectByteBuffer memory >>>>> > > (updateDirectByteBuffer), you're only using 32bit reads. >>>>> > I will add a comment in the code for this decision. The >>>>> reason is >>>>> > that read a long results in slightly worse performance in >>>>> this case, >>>>> > in updateBytes it is faster. I was able to get it to run >>>>> slightly >>>>> > faster by working directly with the address instead of >>>>> always adding >>>>> > address + off, but this makes things worse in the 32bit >>>>> case since >>>>> > all calculation will now be using long variables. So using >>>>> the getInt >>>>> > as in the current code feels like the best solution as it >>>>> strikes the >>>>> > best balance between 32 and 64bit. Below is how >>>>> updateByteBuffer >>>>> > looked with the rewrite I mentioned. >>>>> > >>>>> > >>>>> > ong address = ((DirectBuffer) buffer).address(); >>>>> > crc = updateDirectByteBuffer(crc, address + pos, address >>>>> + limit); >>>>> > >>>>> > >>>>> > private static int updateDirectByteBuffer(int crc, >>>>> long adr, >>>>> > long end) { >>>>> > >>>>> > // Do only byte reads for arrays so short they >>>>> can't be >>>>> > aligned if (end - adr >= 8) { >>>>> > >>>>> > // align on 8 bytes >>>>> > int alignLength = (8 - (int) (adr & 0x7)) & 0x7; >>>>> > for (long alignEnd = adr + alignLength; adr < >>>>> alignEnd; >>>>> > adr++) { crc = (crc >>> 8) >>>>> > ^ byteTable[(crc ^ >>>>> UNSAFE.getByte(adr)) & >>>>> > 0xFF]; } >>>>> > >>>>> > if (ByteOrder.nativeOrder() == >>>>> ByteOrder.BIG_ENDIAN) { >>>>> > crc = Integer.reverseBytes(crc); >>>>> > } >>>>> > >>>>> > // slicing-by-8 >>>>> > for (; adr < (end - Long.BYTES); adr += >>>>> Long.BYTES) { >>>>> > int firstHalf; >>>>> > int secondHalf; >>>>> > if (Unsafe.ADDRESS_SIZE == 4) { >>>>> > // On 32 bit platforms read two ints >>>>> instead of >>>>> > a single 64bit long firstHalf = UNSAFE.getInt(adr); >>>>> > secondHalf = UNSAFE.getInt(adr + >>>>> Integer.BYTES); >>>>> > } else { >>>>> > long value = UNSAFE.getLong(adr); >>>>> > if (ByteOrder.nativeOrder() == >>>>> > ByteOrder.LITTLE_ENDIAN) { firstHalf = (int) value; >>>>> > secondHalf = (int) (value >>> 32); >>>>> > } else { // ByteOrder.BIG_ENDIAN >>>>> > firstHalf = (int) (value >>> 32); >>>>> > secondHalf = (int) value; >>>>> > } >>>>> > } >>>>> > crc ^= firstHalf; >>>>> > if (ByteOrder.nativeOrder() == >>>>> > ByteOrder.LITTLE_ENDIAN) { crc = byteTable7[crc & 0xFF] >>>>> > ^ byteTable6[(crc >>> 8) & 0xFF] >>>>> > ^ byteTable5[(crc >>> 16) & >>>>> 0xFF] >>>>> > ^ byteTable4[crc >>> 24] >>>>> > ^ byteTable3[secondHalf & 0xFF] >>>>> > ^ byteTable2[(secondHalf >>> >>>>> 8) & 0xFF] >>>>> > ^ byteTable1[(secondHalf >>> >>>>> 16) & 0xFF] >>>>> > ^ byteTable0[secondHalf >>> 24]; >>>>> > } else { // ByteOrder.BIG_ENDIAN >>>>> > crc = byteTable0[secondHalf & 0xFF] >>>>> > ^ byteTable1[(secondHalf >>> >>>>> 8) & 0xFF] >>>>> > ^ byteTable2[(secondHalf >>> >>>>> 16) & 0xFF] >>>>> > ^ byteTable3[secondHalf >>> 24] >>>>> > ^ byteTable4[crc & 0xFF] >>>>> > ^ byteTable5[(crc >>> 8) & 0xFF] >>>>> > ^ byteTable6[(crc >>> 16) & >>>>> 0xFF] >>>>> > ^ byteTable7[crc >>> 24]; >>>>> > } >>>>> > } >>>>> > >>>>> > if (ByteOrder.nativeOrder() == >>>>> ByteOrder.BIG_ENDIAN) { >>>>> > crc = Integer.reverseBytes(crc); >>>>> > } >>>>> > } >>>>> > >>>>> > // Tail >>>>> > for (; adr < end; adr++) { >>>>> > crc = (crc >>> 8) >>>>> > ^ byteTable[(crc ^ >>>>> UNSAFE.getByte(adr)) & 0xFF]; >>>>> > } >>>>> > >>>>> > return crc; >>>>> > } >>>>> > >>>>> > >>>>> > > >>>>> > > Also, in updateBytes, the usage of >>>>> > > Unsafe.ARRAY_INT_INDEX_SCALE/ARRAY_LONG_INDEX_SCALE to >>>>> index a byte >>>>> > > array sounds a little scary. To be ultra portable you >>>>> could check >>>>> > > that ARRAY_BYTE_INDEX_SCALE == 1 first and refuse to use >>>>> Unsafe for >>>>> > > byte arrays if it is not 1. Then use >>>>> Integer.BYTES/Long.BYTES to >>>>> > > manipulate 'offsets' instead. In updateDirectByteBuffer >>>>> it would be >>>>> > > more appropriate to use Integer.BYTES/Long.BYTES too. >>>>> > Good idea. Added a check in the initial if statement and it >>>>> will get >>>>> > automatically optimized away. >>>>> > >>>>> > > 225 firstHalf = (int) (value & >>>>> > > 0xFFFFFFFF); 226 secondHalf = (int) (value >>>>> > > >>> 32); 227 } else { // ByteOrder.BIG_ENDIAN >>>>> > > 228 firstHalf = (int) (value >>> 32); >>>>> > > 229 secondHalf = (int) (value & >>>>> > > 0xFFFFFFFF); >>>>> > > >>>>> > > firstHalf = (int) value; // this is equivalent for line 225 >>>>> > > secondHalf = (int) value; // this is equivalent for line >>>>> 229 >>>>> > Done. >>>>> > >>>>> > Here is the latest webrev, >>>>> > http://cr.openjdk.java.net/~sfriberg/JDK-6321472/webrev.03 >>>>> <http://cr.openjdk.java.net/%7Esfriberg/JDK-6321472/webrev.03> >>>>> > >>>>> > Cheers, >>>>> > Staffan >>>>> >>>>> >>>>> >>>> >>>> >>> >> >
