-
Notifications
You must be signed in to change notification settings - Fork 110
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
Adds PresenceBitmap #895
Adds PresenceBitmap #895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This will be easy to integrate with.
fun validate() { | ||
signature.forEachIndexed { i, it -> | ||
val presenceValue = get(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will this be called? Does performance matter? If so, implementing as an iterator rather than repetitively calling get(i) would likely be more efficient, as you could skip a lot of the work that get(i) does each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after seeing amazon-ion/ion-tests#120, I'm not sure whether this method is even useful anymore. Do we want to check how the argument is encoded, or should we check the actual number of expressions produced/provided by the argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies here and for performance of this class in general.
I tried adding an iterator, and then it occurred to me that an Iterator can only hold boxed values, so I looked at the byte code to see what was going on.
For get
:
L2
FRAME SAME
ILOAD 1
BIPUSH 32
IDIV
TABLESWITCH
0: L3
1: L4
2: L5
3: L6
default: L7
L3
FRAME SAME
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap.a : J
GOTO L8
L4
FRAME SAME
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap.b : J
GOTO L8
L5
FRAME SAME
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap.c : J
GOTO L8
L6
FRAME SAME
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap.d : J
GOTO L8
<I've omitted the unreachable default case>
L8
FRAME SAME1 J
LSTORE 2
L9
ILOAD 1
BIPUSH 32
IREM
ICONST_2
IMUL
ISTORE 4
L10
LLOAD 2
ILOAD 4
LSHR
LDC 3
LAND
LRETURN
It looks like a lot, but all of these are very cheap operations. If all macros have 32 parameters or fewer, then the JIT can completely optimize away the TABLESWITCH
, making this a completely branchless set of local load/store operations and math operations.
Using an iterator, on the other hand, requires two INVOKEVIRTUAL
just to get the next long
from the iterator.
L8
ALOAD 2
INVOKEVIRTUAL com/amazon/ion/impl/bin/PresenceBitmap$Iter.next ()Ljava/lang/Long;
INVOKEVIRTUAL java/lang/Long.longValue ()J
LSTORE 7
And in the iterator, in order to produce the next value:
L0
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.i : I
BIPUSH 32
IREM
IFNE L1
L2
ALOAD 0
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.i : I
BIPUSH 32
IDIV
TABLESWITCH
0: L3
1: L4
2: L5
3: L6
default: L7
L3
FRAME SAME1 com/amazon/ion/impl/bin/PresenceBitmap$Iter
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.a : J
GOTO L8
L4
FRAME SAME1 com/amazon/ion/impl/bin/PresenceBitmap$Iter
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.b : J
GOTO L8
L5
FRAME SAME1 com/amazon/ion/impl/bin/PresenceBitmap$Iter
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.c : J
GOTO L8
L6
FRAME SAME1 com/amazon/ion/impl/bin/PresenceBitmap$Iter
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.d : J
GOTO L8
<unreachable default case omitted>
L8
FRAME FULL [com/amazon/ion/impl/bin/PresenceBitmap$Iter] [com/amazon/ion/impl/bin/PresenceBitmap$Iter J]
PUTFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.currentLong : J
L1
FRAME SAME
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.currentLong : J
LDC 3
LAND
LSTORE 1
L9
LINENUMBER 273 L9
ALOAD 0
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.currentLong : J
ICONST_2
LSHR
PUTFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.currentLong : J
L10
ALOAD 0
GETFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.i : I
ISTORE 3
ALOAD 0
ILOAD 3
ICONST_1
IADD
PUTFIELD com/amazon/ion/impl/bin/PresenceBitmap$Iter.i : I
L11
LLOAD 1
INVOKESTATIC java/lang/Long.valueOf (J)Ljava/lang/Long;
ARETURN
There's a lot of similarity. The main thing, though, is that we're having to store the iterator state in the fields of the Iterator instead of the local stack, and there's an extra branch. I could get rid of some of that extra branching and extra state, but then next()
would just be a wrapper around get(i)
(but still with extra boxing of the long
values).
I'm going to leave this as is, and we can revisit it if it does prove to be a bottleneck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I forgot that you can create a private iterator without implementing Iterator
so that it can return long
instead of Long
. That saves us from the auto-boxing, but it still doesn't seem like it's saving much in the way of computation, and it does add the overhead of an extra class.
currentByte = bytes[currentPosition++] | ||
} | ||
val pbValue = ((currentByte.toLong()) shr ((bitmapIndex % PB_SLOTS_PER_BYTE) * PB_BITS_PER_SLOT)) and TWO_BIT_MASK | ||
set(i, pbValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar note here about efficiency. Some of the calculations in set(i) can be removed or simplified if we retain more state here.
@Test | ||
fun `when all parameters are tagged and not exactly-one, should write expected number of presence bits`() { | ||
// Index of an element in this list is the number of parameters in the signature | ||
listOf(0, 0, 0, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss the "2 or more tagged parameters" heuristic in an upcoming sync. We need to be clear how this would be encoded without presence bits.
fun `read 4 parameters`() { | ||
val signature = listOf(taggedZeroToMany, taggedZeroOrOne, taggedExactlyOne, taggedZeroToMany) | ||
|
||
val bytes = bitStringToByteArray("00100010") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by this at first because I was trying to map the four slots here to the four elements of the signature, but only three of the elements of the signature correspond to presence bits, while the exactly-one is implicit. The comment below implies that, but wasn't clear enough for me to get it the first time. Consider adding a comment before this line to explain that the bits are read in pairs from right-to-left and are only present for non-!
signature elements. It would also help if it were bitStringToByteArray("100010")
, assuming this will be automatically zero-padded anyway.
|
||
/** | ||
* The purpose of this utility function is to create a bit string containing a whole number | ||
* of little endian bytes that represents a list of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment trails off
(0 until (((parameterPresences.size + 3) / 4) * 4)).forEach { i -> | ||
// Calculate the little-endian position | ||
val ii = i - 2 * (i % 4) + 3 | ||
// If we go beyond the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment trails off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like my two "comment trails off" comments haven't yet been addressed. Everything else looks good.
Issue #, if available:
This relates to #741 and #742
Description of changes:
Adds a class to help manage presence bitmaps for binary E-Expressions.
In doing so, I also made these incidental changes:
FixedInt
class (actually a singleton with static methods).toString()
representation forMacro.Parameter
that looks like the serialized text representation of a parameter. (Not actually used by the code in this PR, but it was useful while I was debugging things.)uint8
type to theParameterEncoding
enum so that I could test with a tagless type.writeFixedIntOrUIntAt()
method toWriteBuffer
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.