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

Introduce new, better performing IoRingLogDevice2. #155

Merged
merged 11 commits into from
Jul 25, 2024

Conversation

tonyastolfi
Copy link
Collaborator

Instead of using a fixed-size set of flush ops, this impl just flushes the next chunk of data directly and overwrites a single control block on media.

In micro-benchmarks, it performs over 2x as well as the old IoRingLogDevice, while using less memory and being much simpler code-wise. Recovery is also simpler and faster since the control block holds a ring buffer of commit pos values (which are by definition aligned to slot record boundaries).

Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

First couple of comments as I'm getting started on this review. I'll do this PR over the next several working days and submit feedback in chunks.

src/llfs/ioring_log_device.test.hpp Show resolved Hide resolved
src/llfs/ioring_log_device2.cpp Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Show resolved Hide resolved
Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

More comments

src/llfs/ioring_log_device2.hpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Show resolved Hide resolved
src/llfs/ioring_log_device2.hpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.ipp Show resolved Hide resolved
src/llfs/ioring_log_device2.ipp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.ipp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.ipp Outdated Show resolved Hide resolved
Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

Review of log_device2.test.cpp

src/llfs/ioring_log_device2.test.cpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Outdated Show resolved Hide resolved
src/llfs/ioring_log_device2.test.cpp Show resolved Hide resolved
Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

Last part of my review! All done.

src/llfs/log_device_config2.cpp Show resolved Hide resolved
src/llfs/log_device_config2.cpp Show resolved Hide resolved
src/llfs/log_device_config2.hpp Outdated Show resolved Hide resolved
src/llfs/nested_log_device_config.hpp Show resolved Hide resolved
src/llfs/packed_log_control_block2.hpp Show resolved Hide resolved
src/llfs/packed_log_control_block2.hpp Show resolved Hide resolved
src/llfs/packed_log_control_block2.hpp Outdated Show resolved Hide resolved
src/llfs/storage_context.cpp Show resolved Hide resolved
Copy link
Collaborator

@gabrielbornstein gabrielbornstein left a comment

Choose a reason for hiding this comment

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

Good to go! Merge away!

@tonyastolfi tonyastolfi merged commit 8d4ca66 into mathworks:main Jul 25, 2024
4 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