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

Jump to bottom

New tunable --shm-metadata-msg-size #435

Merged
merged 6 commits into from
Jun 13, 2023
Merged

New tunable --shm-metadata-msg-size #435

merged 6 commits into from
Jun 13, 2023

Conversation

dennisklein
Copy link
Member

@dennisklein dennisklein commented Aug 17, 2022 •
edited
Loading

tests/docs and multipart send coming tomorrow, but should already be enough for a first round of discussion.

Implements #432

@dennisklein dennisklein added this to the v1.5 milestone Aug 17, 2022
@dennisklein dennisklein requested a review from rbx August 17, 2022 18:23
@dennisklein dennisklein changed the title Shmem meta size New tunable --shm-metadata-msg-size Aug 17, 2022
@dennisklein dennisklein force-pushed the shmem-meta-size branch 3 times, most recently from 2613938 to b1c8d8d Compare August 17, 2022 18:41
dennisklein
dennisklein commented Aug 17, 2022
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
dennisklein
dennisklein commented Aug 17, 2022
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
rbx
rbx reviewed Aug 17, 2022
View reviewed changes
fairmq/tools/MakeUnique.h Outdated Show resolved Hide resolved
@dennisklein dennisklein force-pushed the shmem-meta-size branch 5 times, most recently from 5c9c611 to 6b508e1 Compare August 22, 2022 10:25
@dennisklein dennisklein linked an issue Aug 22, 2022 that may be closed by this pull request
Add option --shm-metadata-msg-size #432
Closed
@dennisklein dennisklein force-pushed the shmem-meta-size branch 3 times, most recently from 476e4f8 to 2592e2c Compare August 31, 2022 11:40
@dennisklein dennisklein mentioned this pull request Aug 31, 2022
Closed
@dennisklein dennisklein requested a review from rbx August 31, 2022 11:55
dennisklein
dennisklein commented Aug 31, 2022
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
@dennisklein dennisklein force-pushed the shmem-meta-size branch 4 times, most recently from fb01977 to 4cf36a0 Compare September 5, 2022 15:06
@dennisklein dennisklein marked this pull request as ready for review April 20, 2023 10:14
@knopers8
Copy link
Contributor

Thank you!

@dennisklein dennisklein modified the milestones: v1.5, v1.6 Jun 7, 2023
@dennisklein
Copy link
Member Author

dennisklein commented Jun 7, 2023 •
edited
Loading

@knopers8 I haven't been able to push a release until now. However, I think, with the latest update, this is close to become mergeable. Also, @rbx will be back soon and let's see what he is thinking. Since I am on a trip from tomorrow till sunday, I can only continue next Monday. If you have the chance to test this branch here, it would be appreciated.

@rbx I intentionally removed some of the already merged api removal commits in dev from this PR branch with the intent to force-push this to dev once its ready to merge (so, don't use the gh merge button for now). The plan is to release this feature as v1.6.0 to have a minimal diff to v1.5.0, and then make a v2.0.0 release with the deprecated api removals later.

dennisklein
dennisklein commented Jun 12, 2023
View reviewed changes
fairmq/zeromq/Common.h Outdated Show resolved Hide resolved
@dennisklein
Copy link
Member Author

@knopers8 Currently, there is no way to tune this per channel, only per device and applies to all sending shmem channels in that device. Is this granular enough?

rbx
rbx reviewed Jun 12, 2023
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
@knopers8
Copy link
Contributor

@knopers8 Currently, there is no way to tune this per channel, only per device and applies to all sending shmem channels in that device. Is this granular enough?

Yes, that's good enough for the cases I'd like to apply this.

dennisklein
dennisklein commented Jun 12, 2023
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
dennisklein
dennisklein commented Jun 12, 2023
View reviewed changes
fairmq/shmem/Socket.h Outdated Show resolved Hide resolved
@dennisklein dennisklein force-pushed the shmem-meta-size branch 2 times, most recently from a364a10 to 4a73b31 Compare June 13, 2023 06:03
@dennisklein dennisklein requested a review from rbx June 13, 2023 06:07
@dennisklein
feat: Move ZMsg to fair::mq::zmq
79f568b 
Implement move semantics.
dennisklein and others added 4 commits June 13, 2023 16:36
@dennisklein
feat: Use zmq_msg_send for single message Send
12aa04a
@dennisklein
feat: Add new tunable --shm-metadata-msg-size
368d42c 
The shm metadata msg will be right-padded to the given size. This
tunable may be used to saturate the kernel msg buffers more quickly with
the effect that the ZeroMQ message queue size - on which the FairMQ
shmem transport relies upon - behaves more accurately for very small
queue sizes.

This introduces a change for the meta msg format in the multipart case:
old: | MetaHeader 1 | ... | MetaHeader n |
new: | n | MetaHeader 1 | ... | MetaHeader n | padded to fMetadataMsgSize |
where `n` is a `size_t` and contains the number of following meta headers.
Previously, this number was infered from the msg buffer size itself which is
no longer possible due to the potential padding.

Implements FairRootGroup#432
@rbx @dennisklein
test: Add data transfer and checks to protocol tests
73558a1
@rbx @dennisklein
test: Add coverage for --shm-metadata-msg-size
dee0bc5
@dennisklein
Copy link
Member Author

@rbx thx for the test coverage. I shortened the code even more and added the changes from what we discussed earlier today.

@rbx
Copy link
Member

rbx commented Jun 13, 2023

Looks good!

@dennisklein dennisklein merged commit 25614e3 into FairRootGroup:dev Jun 13, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbx rbx rbx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

Add option --shm-metadata-msg-size
4 participants
@dennisklein @rbx @knopers8 @ktf