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.

Sign up for GitHub

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

Logbuffer metadata extra fields #1700

Merged
merged 25 commits into from
Jan 8, 2025
Merged

Logbuffer metadata extra fields #1700

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a378c8f
Initial checking extra properties logbuffer metadata
pveentjer Dec 20, 2024
708407e
WIP
pveentjer Dec 20, 2024
a0b6493
Minor cleanup
pveentjer Dec 20, 2024
ce1040f
Further improvements
pveentjer Dec 20, 2024
0c7a358
Minor fixes
pveentjer Dec 20, 2024
2bd9830
Booleans are now encoded as byte instead of int
pveentjer Dec 20, 2024
57ea62d
Removed the extra cacheline of padding after the frame header
pveentjer Dec 20, 2024
6c4e28e
Fixed checkstyle issues
pveentjer Dec 20, 2024
e6039b3
Moved as many fields as possible before the frame header
pveentjer Dec 30, 2024
b5cf57c
Checkstyle fixes
pveentjer Dec 30, 2024
ee98864
Lot of minor improvements
pveentjer Dec 31, 2024
b092c5f
More work on descriptor
pveentjer Dec 31, 2024
8ee1291
More work log buffer descriptor
pveentjer Dec 31, 2024
a94276a
hacks
pveentjer Jan 2, 2025
84c9146
Disabled the fields after the end of the original record; now java_sy…
pveentjer Jan 2, 2025
37e060e
Added the default_header to the struct
pveentjer Jan 2, 2025
7a7fe9a
All working apart from the assert from aeron_logbuffer_metadata_t size
pveentjer Jan 2, 2025
e4625f3
Fixed padding problem
pveentjer Jan 2, 2025
6066761
Update the order of the fields to reflect master
pveentjer Jan 2, 2025
e177c9c
Furher alignment
pveentjer Jan 2, 2025
015496f
Merge fixes
pveentjer Jan 2, 2025
13264fe
Fixes
pveentjer Jan 2, 2025
c7e8a57
Updated javadoc field layout
pveentjer Jan 3, 2025
1fbfb2c
Processed review comments
pveentjer Jan 7, 2025
7742a73
Removed padding from LogBufferDescriptor documentation
pveentjer Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aeron-client/src/main/c/concurrent/aeron_logbuffer_descriptor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ _Static_assert(
"offsetof(aeron_logbuffer_metadata_t, tether) is wrong");

_Static_assert(
sizeof(aeron_logbuffer_metadata_t) == 480,
sizeof(aeron_logbuffer_metadata_t) == 512,
"sizeof(aeron_logbuffer_metadata_t) is wrong");

_Static_assert(
Expand Down
1 change: 1 addition & 0 deletions aeron-client/src/main/c/concurrent/aeron_logbuffer_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ typedef struct aeron_logbuffer_metadata_stct
uint8_t signal_eos;
uint8_t spies_simulate_connection;
uint8_t tether;
uint8_t pad_end[32]; // Padding to align the structure size to 512 bytes (9x AERON_CACHE_LINE_LENGTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add trailing padding? The entire metadata section is 4KB and is cache line aligned. We are going to read only the fields we need, i.e. consume up to 480 bytes.

Copy link
Contributor Author

@pveentjer pveentjer Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The original code also had padding at the end of the struct:

uint8_t pad3[(AERON_CACHE_LINE_LENGTH) - (7 * sizeof(int32_t))];

My guess is that it was done to prevent false sharing.

Copy link
Contributor Author

@pveentjer pveentjer Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I had another look. I think I understand what is happening.

The padding is needed to position the defaultHeader. It isn't needed to prevent false sharing.

Since the empty space (the pad3) is now filled with the extra fields, the padding isn't relevant any longer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, so no extra padding is needed. The definition on the C side was correct after the 9eb689f commit.

}
aeron_logbuffer_metadata_t;
#pragma pack(pop)
Expand Down
Loading
Loading