Skip to content
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

Implements reading/writing FlexInt, FlexUInt #690

Merged
merged 18 commits into from
Jan 10, 2024
Merged

Implements reading/writing FlexInt, FlexUInt #690

merged 18 commits into from
Jan 10, 2024

Conversation

zslayton
Copy link
Contributor

Implements reading and writing of Ion 1.1 encoding primitives (FlexInt, FlexUInt).

Also adds benchmarks comparing the time needed to read/write 10k integers with that of the corresponding Ion 1.0 encoding primitives (VarInt, VarUInt). Benchmark results on my 2019 16" Intel MBP:
image

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

@zslayton zslayton changed the title Flex int uint Implements reading/writing FlexInt, FlexUInt Dec 19, 2023
Base automatically changed from bin-writer-1_1-skeleton to main December 20, 2023 13:09
Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR tour

@@ -11,7 +11,6 @@ jobs:
if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != 'amazon-ion/ion-rust'
strategy:
matrix:
# See https://github.com/amazon-ion/ion-rust/issues/353
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Issue #353 was fixed in #688, which changed the CI task back to windows-latest.

- name: Remove MSys64 MingW64 Binaries
if: runner.os == 'Windows'
# remove this because there is a bad libclang.dll that confuses bindgen
run: Remove-Item -LiteralPath "C:\msys64\mingw64\bin" -Force -Recurse
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This was necessary for ion-c-sys, which was removed.

use criterion::{black_box, criterion_group, criterion_main, Criterion};
use rand::prelude::StdRng;
use rand::{distributions::Uniform, Rng, SeedableRng};
use std::io;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This file is a suite of benchmarks comparing the time needed to read or write 10k random integers using VarUInt, VarInt, FlexUInt, or FlexInt encoding.

@@ -9,7 +9,7 @@ use crate::IonType;
/// [Typed Value Formats](https://amazon-ion.github.io/ion-docs/docs/binary.html#typed-value-formats)
/// section of the binary Ion spec.
#[derive(Copy, Clone, Debug, PartialEq)]
pub(crate) struct TypeDescriptor {
pub struct TypeDescriptor {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This PR makes a couple of types pub while leaving their module pub(crate) or even private. A later change in this diff re-exports these pub types if the experimental-lazy-reader feature is enabled, allowing the benchmarks to use some of these implementation details.

lazy::encoder::binary::v1_1::flex_int::FlexInt,
lazy::encoder::binary::v1_1::flex_uint::FlexUInt,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Feature-gated re-exports of encoding-primitive-related types.

Comment on lines 220 to 224
// TODO: Should this be a const cache of num_encoded_bits -> mask?
let mask = 1u64
.checked_shl(num_encoded_bits as u32)
.map(|v| v - 1)
.unwrap_or(u64::MAX);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ I think making a const cache for this would get us a modest speedup, but the performance is already much better than VarUInt.

@zslayton zslayton requested review from popematt and tgregg December 20, 2023 19:56
src/lazy/binary/immutable_buffer.rs Outdated Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Outdated Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Outdated Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Show resolved Hide resolved
src/lazy/binary/immutable_buffer.rs Show resolved Hide resolved
src/lazy/binary/immutable_buffer.rs Show resolved Hide resolved
src/lazy/binary/immutable_buffer.rs Outdated Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Outdated Show resolved Hide resolved
src/lazy/encoder/binary/v1_1/flex_uint.rs Show resolved Hide resolved
@zslayton zslayton requested a review from popematt January 10, 2024 15:13
@zslayton zslayton merged commit c109160 into main Jan 10, 2024
20 checks passed
@zslayton zslayton deleted the flex-int-uint branch January 10, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants