Repository navigation
Add handlers for null, typed null, and boolean opcodes - #1112
Conversation
| opcode: Int, | ||
| /** Byte buffer containing the source binary data, or a part of it */ | ||
| source: ByteArray, | ||
| /** The position in [source] of the current opcode - that is, `source[position] == opcode` */ |
There was a problem hiding this comment.
Suggestion—I think you may find that it streamlines things a bit if you make this point to the next unread byte in the data stream. (Usually, the first byte after the opcode.)
Consider the case of tagless values. They aren't preceded by an opcode—rather their opcode is part of the template definition or tagless-element sequence—so source[position] == opcode isn't guaranteed to hold.
|
It looks like one of the automated checks is failing because of a code style issue. (I know, the performance regression tests add a lot of noise.) Running |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ion11 #1112 +/- ##
========================================
Coverage ? 68.11%
Complexity ? 5724
========================================
Files ? 186
Lines ? 23872
Branches ? 4232
========================================
Hits ? 16260
Misses ? 6314
Partials ? 1298 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
popematt
left a comment
popematt
left a comment
There was a problem hiding this comment.
Overall, it looks good. I've left a couple of observations about potential performance improvements, but let's not worry about digging into those right now.
There's a comment that needs to be removed (because you fixed the thing it was talking about), and then I think it's ready to be merged.
| * @return The number of additional bytes that had to be read off of [source] to handle this opcode. Should be zero | ||
| * if it was possible to handle the opcode without reading any additional data. | ||
| */ | ||
| // TODO: this will also need to accept the constant pool, symbol table, and macro table for certain opcodes |
There was a problem hiding this comment.
Don't forget to remove the comment if you've resolved this already.
| if (typeOperand < 0 || typeOperand >= typeTable.size) { | ||
| throw IonException("Unsupported typed null") | ||
| } |
There was a problem hiding this comment.
Don't do anything with this comment. I'm leaving this comment to aid my own recollection later.
We should see whether it's faster in the happy case to check this condition or to just catch an ArrayIndexOutOfBoundsException and rethrow as an IonException. I've created #1114 to go back and revisit this issue later.
| val ionType = typeTable[typeOperand] | ||
| BytecodeEmitter.emitNullValue(destination, ionType) |
There was a problem hiding this comment.
Suggestion—I'm not opposed to using things that might be slightly less readable here in order to improve performance. In this case, we're converting from an integer (the type operand, as you've named it) to an IonType object instance, and then in the BytecodeEmitter it's converting from the IonType back into an integer. I think that we should be able to create a function that converts from the typeOperand to the typed null instruction without having to use a lookup table to go via IonType.
Something like this, maybe:
private fun nullInstructionForTypedNull(nullType: Int): Int {
// See https://amazon-ion.github.io/ion-docs/books/ion-1-1/binary/values/null.html
// and instruction_reference.md
// Add 1 to go from nullType to OperationKind for the appropriate Ion type
// Then shift left 27, and finally add in the `111` (or `0x7`) operation kind bits.
return (nullType + 1).shl(27).or(0x07000000)
}| import org.junit.jupiter.params.ParameterizedTest | ||
| import org.junit.jupiter.params.provider.CsvSource | ||
|
|
||
| class TypedNullOpcodeHandlerTest { |
There was a problem hiding this comment.
This is very clean, concise, and to-the-point. Nice!
| macroIndices: IntArray, | ||
| symbolTable: Array<String?> | ||
| ): Int { | ||
| BytecodeEmitter.emitBoolValue(destination, opcode == 0x6E) |
There was a problem hiding this comment.
Just an observation. Don't make any changes based on this comment.
It's not immediately obvious, but there is some redundant branching going on here.
- When getting the handler from the lookup table, we are effectively branching on the opcode.
- Here we convert the opcode to either
trueorfalse - Then in
BytecodeEmitter, we haveif (bool) 1 else 0.
We could solve this by having a true handler and a false handler, or by performing some arithmetic to get the right value, but I actually think this is an indicator that the spec could be slightly more ergonomic. If we swapped the opcodes for true and false, then we could do something really simple like this:
destination.add(I_BOOL.packInstructionData(opcode - OpCode.BOOL_FALSE))
2 participants
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.