Adds support for macros to IonRawBinaryWriter_1_1 #668

popematt · 2023-12-12T01:02:07Z

Issue #, if available:

None

Description of changes:

Updates the signature of stepInEExp() and implements it.
There are a couple of incidental changes that I've called out with a 🗺️ comment.

~~Potentially controversial... this PR uses the end delimiter to signal the end of a macro's args if that macro has rest-parameter in it's signature.~~

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

popematt · 2023-12-12T01:03:06Z

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

@@ -55,7 +55,7 @@ class IonRawBinaryWriter_1_1 internal constructor(
        /**
         * Clears this [ContainerInfo] of old data and initializes it with the given new data.
         */
-        fun reset(type: ContainerType? = null, position: Long = -1, isDelimited: Boolean = false) {
+        fun reset(type: ContainerType, position: Long, isDelimited: Boolean = false) {


🗺️ Incidental change—I realize that the default values are always supplied for both type and position, so I removed the default values.

popematt · 2023-12-12T01:04:37Z

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

@@ -448,6 +467,9 @@ class IonRawBinaryWriter_1_1 internal constructor(
                List, SExp, Struct -> {
                    val contentLength = currentContainer.length
                    if (contentLength <= 0xF && !currentContainer.usesFlexSym) {
+                        // TODO: Right now, this is skipped if we switch to FlexSym after starting a struct


🗺️ This is incidental. I was aware of this when I implemented structs, but I had since forgotten it, and then I rediscovered this while I was writing tests for E-Exps.

Can you provide a little more detail here? What are the implications? That we can't reclaim space for FlexSym structs? Why not?

The scenario is when we have a struct that starts out as a SID struct, has at least one SID field before switching to FlexSyms, and has a total length of less than 16 bytes. The consequence of this limitation is that we're wasting a byte. E.g.:

FC | Variable length SID Struct ]____ Could be replaced with 17 | Length = 11 ] just `CB` to save a byte. 81 | SID 64 5E | true 01 | switch to FlexSym encoding FB 66 6F 6F | FlexSym 'foo' 5E | true 02 01 | FlexSym SID 64 5E | true

Ideally, we could write this starting with CB but we don't store enough state right now to track whether it's safe to do that. We only know that the struct is currently using FlexSyms, which means that we don't know whether the OpCode at the start is FC or FD. If it's FC, then it's safe to collapse FC 17 to CB (as in the example above). If it actually started with FD, then going to CB would cause the struct to be malformed.

It's not difficult to solve this—it just requires a way to check how the struct started out and a few more branches for the different scenarios—but I didn't really want to address it while the struct encoding is still in a bit of flux.

popematt · 2023-12-12T01:05:11Z

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

+        // Instead of allocating iterator, we share one iterator instance within the scope of the container stack and
+        // reset the cursor every time we track back to the ancestors.


🗺️ Incidental formatting change so that the line isn't ridiculously long.

popematt · 2023-12-13T05:47:37Z

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java

@@ -1044,12 +1044,12 @@ private void patchSingleByteTypedOptimisticValue(final byte type, final Containe
        if (info.length <= 0xD)
        {
            // we fit -- overwrite the type byte
-            buffer.writeUInt8At(info.position - 1, type | info.length);
+            buffer.writeByteAt(info.position - 1, type | info.length);


🗺️ The writeUInt8At method got renamed to writeByteAt because I found it misleading to use writeUInt8 to write FlexInt.ZERO. I checked all usages of the method, and the new name writeByteAt works for all of them. None of the existing usages were specifically trying to write an 8-bit integer; rather they were writing either a sentinel (such as an op code) or 8 bits of a larger value.

popematt · 2023-12-13T06:01:29Z

src/main/java/com/amazon/ion/impl/bin/WriteBuffer.java

+        while (this.index != index) {
+            blocks.remove(this.index--);
+        }


How long has this bug existed? Does it require patching in master and/or previous versions?

I think it's been there all along, but I haven't looked into the potential impact. Since it's been there all along, it's even possible that the current implementation has a way to avoid the buggy behavior by cleaning the blocks at some other time.

popematt · 2023-12-13T06:03:11Z

src/main/java/com/amazon/ion/impl/bin/WriteBuffer.java

@@ -1300,7 +1301,11 @@ public void writeVarUIntDirect2At(long position, long value)
        block.data[offset + 1] = (byte) ((value                            & VAR_INT_MASK) | VAR_INT_FINAL_OCTET_SIGNAL_MASK);
    }

-    public void writeUInt8At(final long position, final long value)
+    public void writeByteAt(final long position, final byte value) {


🗺️ Convenience overload. If it's a perf problem we can remove it, but I bet the JIT is smart enough to optimize this out.

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

tgregg · 2023-12-15T00:32:49Z

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

@@ -448,6 +467,9 @@ class IonRawBinaryWriter_1_1 internal constructor(
                List, SExp, Struct -> {
                    val contentLength = currentContainer.length
                    if (contentLength <= 0xF && !currentContainer.usesFlexSym) {
+                        // TODO: Right now, this is skipped if we switch to FlexSym after starting a struct


Can you provide a little more detail here? What are the implications? That we can't reclaim space for FlexSym structs? Why not?

Co-authored-by: Tyler Gregg <[email protected]>

codecov · 2023-12-15T05:23:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (ion-11-encoding@0255c1a). Click here to learn what that means.

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding     #668   +/-   ##
==================================================
  Coverage                   ?   65.05%           
  Complexity                 ?     5828           
==================================================
  Files                      ?      200           
  Lines                      ?    25029           
  Branches                   ?     4468           
==================================================
  Hits                       ?    16282           
  Misses                     ?     7433           
  Partials                   ?     1314

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

popematt commented Dec 12, 2023

View reviewed changes

popematt requested a review from tgregg December 12, 2023 01:09

popematt commented Dec 13, 2023

View reviewed changes

popematt added 5 commits December 14, 2023 16:53

Adds support for macros to IonRawBinaryWriter_1_1

2db9a46

Adds missing test case

c2774ef

Adds Ion 1.1 raw binary writer support for expression groups

a8d1653

Stops a class from KotlinStdLib from leaking into the public API

08ff18c

Clean up rebase mistake

77b5f5e

popematt force-pushed the ion11-macros branch from 5032e52 to 77b5f5e Compare December 15, 2023 01:00

tgregg approved these changes Dec 15, 2023

View reviewed changes

popematt and others added 2 commits December 14, 2023 21:09

Adds suggested change

0239f79

Co-authored-by: Tyler Gregg <[email protected]>

Fix more rebase mistakes

195ca42

popematt merged commit b3417e2 into amazon-ion:ion-11-encoding Dec 15, 2023
8 of 32 checks passed

popematt deleted the ion11-macros branch December 15, 2023 18:18

tgregg pushed a commit that referenced this pull request Jan 22, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

908a52b

tgregg pushed a commit that referenced this pull request Feb 10, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

c35e2d3

tgregg pushed a commit that referenced this pull request May 2, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

a365bfb

tgregg pushed a commit that referenced this pull request Jun 28, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

a9ba846

tgregg pushed a commit that referenced this pull request Sep 9, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

f29522b

tgregg pushed a commit that referenced this pull request Dec 13, 2024

Adds support for macros to IonRawBinaryWriter_1_1 (#668)

2b02cea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for macros to IonRawBinaryWriter_1_1 #668

Adds support for macros to IonRawBinaryWriter_1_1 #668

popematt commented Dec 12, 2023 •

edited

Loading

popematt Dec 12, 2023

popematt Dec 12, 2023

tgregg Dec 15, 2023

popematt Dec 15, 2023

popematt Dec 12, 2023

popematt Dec 13, 2023

popematt Dec 13, 2023

tgregg Dec 15, 2023

popematt Dec 15, 2023

popematt Dec 13, 2023

tgregg Dec 15, 2023

codecov bot commented Dec 15, 2023

		// Instead of allocating iterator, we share one iterator instance within the scope of the container stack and
		// reset the cursor every time we track back to the ancestors.

Adds support for macros to IonRawBinaryWriter_1_1 #668

Adds support for macros to IonRawBinaryWriter_1_1 #668

Conversation

popematt commented Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2023

Codecov Report

popematt commented Dec 12, 2023 •

edited

Loading