Implement Ion 1.0 byte-array-backed bytecode generator #1143
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ion11 #1143 +/- ##
========================================
Coverage ? 69.97%
Complexity ? 6595
========================================
Files ? 211
Lines ? 26554
Branches ? 4596
========================================
Hits ? 18582
Misses ? 6533
Partials ? 1439 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // TODO(perf): See if there's a way to do this without allocating new ByteBuffers, that is compatible with JDK 8. | ||
| val buffer = ByteBuffer.wrap(source, position, length) |
There was a problem hiding this comment.
You should just be able to wrap the source once, then change its position and limit here.
There was a problem hiding this comment.
There's something weird going on with those methods. I can see them in the JDK 8 javadoc, but I think later JDKs have ByteBuffer override the base method in Buffer, and it was causing issue in the tests running on JDK 8. I'll revisit it and see if I can fix the problem some other way.
There was a problem hiding this comment.
Huh... I followed the reusable ByteBuffer pattern in the continuable reader. Not sure what would be different here.
There was a problem hiding this comment.
It could possibly have to do the difference between invoking the method on the parent class vs on the child class.
| if (TypeIdHelper.isNull(typeId)) { | ||
| compileNullValue(typeId, dest) | ||
| } else when (TypeIdHelper.operationKindForTypeId(typeId)) { | ||
| OperationKind.UNSET -> if (isAnnotated) throw IonException("Invalid annotation wrapper: NOP pad may not occur inside an annotation wrapper.") |
There was a problem hiding this comment.
Is the relevant error message to all situations where this can occur?
There was a problem hiding this comment.
I believe it is relevant any time you have an NOP for which isAnnotated is true. Is there some other case that I'm missing here?
There was a problem hiding this comment.
I guess my question was about whether UNSET is only used for NOP. I think you're saying that it is, and therefore the error message is correct.
There was a problem hiding this comment.
Yes, it is. I'm using -1 for "not a valid type id", so there is nothing else here that is using UNSET. It might be worth revisiting this later to see if we can rename it NOP.
| I_STRUCT_START withData 6, | ||
| I_FIELD_NAME_SID withData 4, | ||
| I_NULL_NULL, | ||
| I_FIELD_NAME_SID withData 5, |
There was a problem hiding this comment.
I'm assuming the field name associated with the no-op is present here because it's faster not to add a check to elide it in this rare case?
2 participants
Description of changes:
Adds Ion 1.0 bytecode generator for a fixed-sized, in-memory data stream.
Previous benchmarks suggest that this may provide ~10% more throughput than the current continuable reader for the use-case that this bytecode generator supports.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.