stevedlawrence commented on a change in pull request #163: Fix performance
issues with recent hexBinary changes
URL: https://github.com/apache/incubator-daffodil/pull/163#discussion_r247885728
##########
File path:
daffodil-io/src/main/scala/org/apache/daffodil/io/InputSourceDataInputStream.scala
##########
@@ -156,170 +156,125 @@ final class InputSourceDataInputStream private (val
inputSource: InputSource)
/**
* Accepts a preallocated array and a bitLength. Reads the specified number
- * of bits and stores them in the array in big endian byte order and most
- * significant bit first bit order. The most significant byte is stored in
- * the zero'th index in the array. This means that if the array is larger
- * than the number of bytes needed for the specified number of bits, the
- * trailing bytes will be untouched and should be ignored by the caller.
+ * of bits and stores them in the array based on byteOrder and bitOrder,
+ * resulting in an array consistent with hexBinary data. If there are
+ * fragment bytes the last bits are placed appropriately based on bitOrder.
+ * For example, assuming a bitLength of 19, the resulting array will be
+ * filled like so,
+ *
+ * BE MSBF: 01234567 01234567 012xxxxx
+ * BE LSBF: 76543210 76543210 xxxxx210
+ *
+ * Little endian is the exact same, but with the bytes reversed, like so:
+ *
+ * LE MSBF: 012xxxxx 01234567 01234567
+ * LE LSBF: xxxxx210 76543210 76543210
+ *
+ * In both cases, the x's are always zeros. It is the responsiblity of the
+ * caller to be aware of the length of the data and to know which bits are
+ * data vs padding based on bitOrder/byteOrder
*/
private def fillByteArray(array: Array[Byte], bitLengthFrom1: Int, finfo:
FormatInfo): Unit = {
- if (isAligned(8) && bitLengthFrom1 % 8 == 0) {
- fillByteArrayAlignedNoFragment(array, bitLengthFrom1, finfo)
- } else {
- fillByteArrayUnalignedOrFragment(array, bitLengthFrom1, finfo)
- }
- }
-
- private def fillByteArrayAlignedNoFragment(array: Array[Byte],
bitLengthFrom1: Int, finfo: FormatInfo): Unit = {
- // threadCheck()
- Assert.usage(isAligned(8))
- Assert.usage(bitLengthFrom1 % 8 == 0)
-
- val bytesToFill = bitLengthFrom1 / 8
- Assert.invariant(array.size >= bytesToFill)
+ val isUnaligned = !isAligned(8)
+ val fragmentBits = bitLengthFrom1 % 8
+ val bytesToFill = (bitLengthFrom1 + 7) / 8
- if (bytesToFill == 1 || // 1 byte is super common case. We don't want to
retrieve byteOrder nor bitOrder in this case
- (finfo.byteOrder == ByteOrder.BigEndian && finfo.bitOrder ==
BitOrder.MostSignificantBitFirst)) {
- // bits & bytes are already in order, read them straight into the array
- inputSource.get(array, 0, bytesToFill)
- } else {
- // we are either LittleEndian & MSBF or BigEndian & LSBF. In either case,
- // we just need to flip the bytes to make it BigEndian MSBF. The bits are
- // in the correct order.
- var i = bytesToFill - 1
- while (i >= 0) {
- array(i) = inputSource.get().toByte
- i -= 1
+ // We may only need N bytes to fill in the array, but if we are unaligned
+ // then that means we could potentially need to get an extra byte. For
+ // example, say bitLength is 8, so we only need 1 byte total and the array
+ // size is 1. If we are unaligned and our bit offset is 1, we need to read
+ // two bytes total, extracting 7 bits from the first byte and 1 bit from
+ // the second byte. To be efficient, let's fill the array with data we know
+ // we'll need. Later, if we determine that we are unaligned and an extra
+ // byte is needed, we will read one more byte into an overflow variable and
+ // eventually combine that into the array.
+ inputSource.get(array, 0, bytesToFill)
+
+ if (isUnaligned) {
+ // If we are not aligned, then we need to shift all the bits we just got
+ // to the left, based on the bitOffset and the bitOrder. Do that shift
+ // here.
+
+ val isMSBF = finfo.bitOrder == BitOrder.MostSignificantBitFirst
+ val bitOffset0b = (bitPos0b % 8).toInt
+
+ // Determine if we need an overflow byte and read it if so
+ val bytesToRead = (bitLengthFrom1 + bitOffset0b + 7) / 8
+ val arrayOverflow = if (bytesToRead > bytesToFill)
Bits.asUnsignedByte(inputSource.get()) else 0
+
+ // Determine the masks and shifts needed to create a new byte based on
+ // the bitOrder
+ val curBitMask =
+ if (isMSBF)
+ Bits.maskR(8 - bitOffset0b)
+ else
+ Bits.maskL(8 - bitOffset0b)
+
+ val nxtBitMask = ~curBitMask & 0xFF
+ val curShift = bitOffset0b
+ val nxtShift = 8 - bitOffset0b
+
+ @inline
Review comment:
Not sure where I learned this trick, but it's not actually a trick. I've
just tested with and without the inline on both scala 2.11 and 2.12, and the
results are all the same, but different that what I thought would happen. In
all cases, there is never an allocation for the closure. Instead, Scala creates
a private static function with the appropriate number of parameters and passes
them all to the static function when called. There is no allocation, but there
is also nothing inlined. The @inline and associated comments can just be
removed. I'd rather keep this function nested and look like a closure since
scala always does the same thing, and expanding it by hand results in a 7
parameter function, making it harder to read.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
