Support for encoding directives, arg groups, length-prefixed e-expressions #796

zslayton · 2024-07-09T21:20:23Z

Adds support for switching encodings midstream
Adds support for parameter cardinality modifiers
Adds support for reading argument groups
Adds support for length-prefixed e-expressions
Optimizes and simplifies macro evaluation
Simplifies struct expansion logic
Adds static analysis optimizations to template evaluation
Adds support for lazily evaluating top-level e-expressions
Adds more Ion 1.1 encodings to the read_many_structs benchmark
Shrinks many data types

* Adds support for switching encodings midstream * Adds support for parameter cardinality modifiers * Adds support for reading argument groups * Adds support for length-prefixed e-expressions * Optimizes and simplifies macro evaluation * Simplifies struct expansion logic * Adds static analysis optimizations to template evaluation * Adds support for lazily evaluating top-level e-expressions * Adds more Ion 1.1 encodings to the `read_many_structs` benchmark * Shrinks many data types Switching encodings midstream, initial support for encoding directives ParameterCardinality, transcribe raw binary 1.1 to text, version switching refactor transcription, groundwork for variadic params Adds non-required cardinalities Read support for binary arg grouping bitmap binary reader support for arg groups minor optimizations allows benchmarks to manually register templates reworked e-expr parsing Binary Ion v1.1 uses &'top for value, eexp types minor optimizations 2 clippy suggestions per-type read paths for string, int better Reworks EncodedValue offset calculation to save on size Adds support for length-prefixed e-exprs simplified struct iteration, rewrote eexp parsing further streamlining of read_sequence_value_expr Optimization for singleton template macros Reorganizes read_many_structs benchmark OneShotMacroEvaluator, EExpArgIterator trait, fixed ion-tests integration upgradeable MacroEvaluator (OneShot -> Stacked) Using new macro evaluator at root level Struct expansion uses new macro evaluator Optimizations for macro expansion * Adds `read_resolved` method to the LazyExpandedValue level so all Ion-type specific logic can be handled once instead of twice. * `MacroExpansion::initialize` now delegates the entire implementation the invocation's source kinds instead of dispatching on every method call. * Universal singleton macro expansion optimization support. Revised TemplateCompiler storage Shrank macro evaluator sizes simplified macro evaluation logic +"optimal path" eexp benchmark, more method inlining s/quote/literal/, clean up read_many_structs benchmark remove erroneous profile json Adds try_next! macro for Option<Result<_>> early returns cleanup clippy suggestions fix doc links more clippy suggestions

zslayton · 2024-07-09T21:21:18Z

Miri is failing due to #794. I'd like to investigate that after this PR is merged.

zslayton

🗺️ PR Tour 🧭

zslayton · 2024-07-09T21:22:54Z

Cargo.toml

@@ -21,7 +21,7 @@ edition = "2021"
 # We need at least 1.65 for GATs[1] and 1.67 for `ilog`[2]
 # [1] https://blog.rust-lang.org/2022/11/03/Rust-1.65.0.html
 # [2] https://blog.rust-lang.org/2023/01/26/Rust-1.67.0.html#stabilized-apis
-rust-version = "1.67"
+rust-version = "1.77"


🗺️ Bumping this again while we still can--1.77 added some nice const generics functionality for breaking apart byte arrays.

while we still can

There's a cfg_version RFC that will help with this.

Also, if we haven't already done so, we should define a MSRV policy for this crate.

zslayton · 2024-07-09T21:24:12Z

benches/read_many_structs.rs

@@ -3,27 +3,262 @@ use criterion::{criterion_group, criterion_main};
 #[cfg(not(feature = "experimental"))]
 mod benchmark {
    use criterion::Criterion;
+
    pub fn criterion_benchmark(_c: &mut Criterion) {


🗺️ I've added a variety of binary 1.1 encodings to the read_many_structs benchmark.

zslayton · 2024-07-09T21:26:02Z

benches/read_many_structs.rs

+    let text_1_1_data = r#"(:event 1670446800245 418 "scheduler-thread-6" "example-client-1" "aws-us-east-5f-18b4fa" (: "region 4" "2022-12-07T20:59:59.744000Z"))"#.repeat(num_values);
+    let mut binary_1_1_data = vec![0xE0u8, 0x01, 0x01, 0xEA]; // IVM
+    #[rustfmt::skip]
+    let mut binary_1_1_data_body: Vec<u8> = vec![


🗺️ The binary writer doesn't yet support argument groups, so I've hand-encoded the binary for now.

src/element/sequence.rs

zslayton · 2024-07-09T22:40:54Z

src/lazy/any_encoding.rs

@@ -126,13 +134,23 @@ impl<'top> HasRange for LazyRawAnyVersionMarker<'top> {
 }

 impl<'top> RawVersionMarker<'top> for LazyRawAnyVersionMarker<'top> {


🗺️ In order to support switching encodings mid-stream, I reworked the RawVersionMarker trait and its impls. I'll call it out again further on. The version() method now returns an IonVersion enum, while major_minor() returns the (major, minor) version tuple.

zslayton · 2024-07-10T16:43:26Z

src/lazy/text/raw/v1_1/arg_group.rs

+use crate::{Decoder, HasRange, HasSpan, IonResult, LazyExpandedValue, Span};
+
+#[derive(Copy, Clone, Debug)]
+pub struct EExpArg<'top, D: Decoder> {


🗺️ These encoding-agnostic types should also be in their own file/module.

zslayton · 2024-07-10T16:44:07Z

src/lazy/text/raw/v1_1/reader.rs

-                    "Ion version {major}.{minor} is not supported"
-                ));
-            }
-        }


🗺️ This is now supported; the logic lives in the LazyAnyRawReader.

zslayton · 2024-07-10T16:44:54Z

src/lazy/value.rs

-            }
-        };
-        Ok(value_ref)
+        self.expanded_value.read_resolved()


🗺️ As described earlier, putting the resolution logic closer to the read logic allowed the compiler to make much better optimizations.

zslayton · 2024-07-10T16:45:36Z

src/lib.rs

+            lazy::expanded::compiler::TemplateCompiler,
+            lazy::expanded::template::TemplateMacro,
+            lazy::expanded::template::TemplateBodyExpr,
+            lazy::expanded::template::TemplateBodyExprKind,
+            lazy::expanded::macro_table::Macro,
+            lazy::expanded::macro_evaluator::MacroEvaluator,
+            lazy::expanded::macro_evaluator::MacroExpansionKind,
+            lazy::expanded::macro_table::MacroKind,
+            lazy::expanded::macro_table::MacroTable,
+            lazy::expanded::EncodingContext,
+            lazy::any_encoding::IonVersion,
+            lazy::binary::raw::reader::LazyRawBinaryReader_1_0,
+            lazy::binary::raw::v1_1::reader::LazyRawBinaryReader_1_1,
+            lazy::expanded::macro_evaluator::RawEExpression,
+            lazy::expanded::macro_evaluator::ValueExpr,
+            lazy::expanded::macro_evaluator::MacroExpr,
+            lazy::expanded::macro_evaluator::MacroExprKind,


🗺️ Additional types exposed behind the experimental-reader-writer feature.

zslayton · 2024-07-10T16:46:36Z

tests/ion_data_consistency.rs

-        .map(|p| p.replace('/', &PATH_SEPARATOR.to_string()))
+        .map(|p| p.replace('/', PATH_SEPARATOR))


🗺️ (wise) clippy suggestion.

popematt · 2024-07-10T17:35:19Z

Cargo.toml

@@ -21,7 +21,7 @@ edition = "2021"
 # We need at least 1.65 for GATs[1] and 1.67 for `ilog`[2]
 # [1] https://blog.rust-lang.org/2022/11/03/Rust-1.65.0.html
 # [2] https://blog.rust-lang.org/2023/01/26/Rust-1.67.0.html#stabilized-apis
-rust-version = "1.67"
+rust-version = "1.77"


while we still can

There's a cfg_version RFC that will help with this.

Also, if we haven't already done so, we should define a MSRV policy for this crate.

benches/read_many_structs.rs

src/lazy/any_encoding.rs

popematt · 2024-07-10T18:02:22Z

src/lazy/any_encoding.rs

+}
+
+#[derive(Debug, Default, Copy, Clone, PartialEq)]
+pub enum IonVersion {


This seems like a common/utility type that should be exported from the crate root, and maybe even be defined in the root module so that it's easily discoverable.

This is re-exported at the root, but I agree it should live in its own module and/or at the root. I'd like to leave that for another PR though.

src/lazy/any_encoding.rs

src/lazy/expanded/template.rs

src/lazy/expanded/compiler.rs

src/lazy/expanded/macro_evaluator.rs

…ase` fn

popematt

Approving so that we can review PR feedback changes in smaller PRs.

zslayton requested a review from popematt July 9, 2024 21:20

s/RawBinaryEExpression_1_1/BinaryEExpression_1_1/, minor cleanup

7655771

zslayton force-pushed the encoding-directives branch from 1ca6a53 to 7655771 Compare July 10, 2024 11:10

zslayton added 5 commits July 10, 2024 08:49

Simplifies read_int

3591bba

remove unused specialized methods in LazyRawValue trait

5b16f51

improved template compiler comments

75f17f2

Simplify making new eval environments for e-exps

cd32f1b

improved macro evaluator comments

9b87e8a

zslayton commented Jul 10, 2024

View reviewed changes

zslayton added 3 commits July 10, 2024 19:57

Switch struct iter back to source when macro eval is done

37f62fb

Removes some generics from ExpandedStructIterator

832eb84

further simplify struct expansion

85450d3

popematt reviewed Jul 11, 2024

View reviewed changes

zslayton mentioned this pull request Jul 23, 2024

Define an MSRV policy for the crate #800

Open

zslayton added 4 commits July 23, 2024 13:36

Incorporates feedback

111796a

Merge remote-tracking branch 'origin/main' into encoding-directives

e3f9032

Adds comment to text buffer PartialEq, moves `read_resolved_general_c…

f60e9f8

…ase` fn

Fixed inner fn definition

189d326

zslayton mentioned this pull request Jul 30, 2024

Adds the (annotate ...) form and bin read support for flex_uint parameters #801

Merged

popematt approved these changes Jul 31, 2024

View reviewed changes

zslayton merged commit b245172 into main Jul 31, 2024
29 of 32 checks passed

zslayton deleted the encoding-directives branch July 31, 2024 00:56

zslayton mentioned this pull request Aug 9, 2024

Adds additional tooling APIs to enable Ion 1.1 support in the CLI's inspect command #802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for encoding directives, arg groups, length-prefixed e-expressions #796

Support for encoding directives, arg groups, length-prefixed e-expressions #796

zslayton commented Jul 9, 2024

zslayton commented Jul 9, 2024

zslayton left a comment

zslayton Jul 9, 2024

popematt Jul 10, 2024

zslayton Jul 9, 2024

zslayton Jul 9, 2024

zslayton Jul 9, 2024

zslayton Jul 10, 2024

zslayton Jul 10, 2024

zslayton Jul 10, 2024

zslayton Jul 10, 2024

zslayton Jul 10, 2024

popematt Jul 10, 2024

popematt Jul 10, 2024

zslayton Jul 23, 2024

popematt left a comment

		@@ -126,13 +134,23 @@ impl<'top> HasRange for LazyRawAnyVersionMarker<'top> {
		}

		impl<'top> RawVersionMarker<'top> for LazyRawAnyVersionMarker<'top> {

		.map(\|p\| p.replace('/', &PATH_SEPARATOR.to_string()))
		.map(\|p\| p.replace('/', PATH_SEPARATOR))

Support for encoding directives, arg groups, length-prefixed e-expressions #796

Support for encoding directives, arg groups, length-prefixed e-expressions #796

Conversation

zslayton commented Jul 9, 2024

zslayton commented Jul 9, 2024

zslayton left a 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

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

popematt left a comment

Choose a reason for hiding this comment