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

Add support for the 8-byte long timestamp delta encoding #166

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

LinZhihao-723
Copy link
Member

@LinZhihao-723 LinZhihao-723 commented Sep 24, 2023

Description

The current IR encoding protocol doesn't support 8-byte timestamp delta encoding. This means the current four-byte encoding IR stream cannot contain a timestamp delta of more than +/- 24.86 days. This PR resolves this problem by introducing an 8-byte timestamp delta encoding.
The protocol version number is bumped to "v0.0.1". As a result, the version of the decoder must have been up-to-date to decode an 8-byte encoded timestamp delta properly.
To validate the version, a validation function is added to check whether the built protocol version number is greater or equal to the input protocol version number. The current version validation relies on the SemVar https://semver.org/#backusnaur-form-grammar-for-valid-semver-versions convention.

Validation performed

  • Added unit tests to cover the timestamp delta encoding for all integer sizes we support. Negative timestamp deltas are also tested.
  • Tested the version number validation:
    • Ensured the old version (v0.0.0) can be compressed successfully.
    • Ensured the version that doesn't follow the SemVer convention will be caught.

@LinZhihao-723 LinZhihao-723 marked this pull request as ready for review September 24, 2023 05:20
@kirkrodrigues kirkrodrigues self-requested a review September 26, 2023 04:43
components/core/src/ffi/ir_stream/protocol_constants.hpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/protocol_constants.hpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.cpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.cpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.hpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_encoding_methods.cpp Outdated Show resolved Hide resolved
components/core/tests/test-ir_encoding_methods.cpp Outdated Show resolved Hide resolved
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Generally we should try to do unrelated formatting changes in another PR so that the changes in this PR concisely match their purpose, but it's not worth changing things back for this one.

components/core/src/ffi/ir_stream/decoding_methods.hpp Outdated Show resolved Hide resolved
components/core/src/ffi/ir_stream/decoding_methods.hpp Outdated Show resolved Hide resolved
@kirkrodrigues kirkrodrigues merged commit cb6058c into y-scope:main Sep 29, 2023
5 checks passed
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