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

SHM lazy init is now configured in config.json #1756

Merged
merged 4 commits into from
Feb 5, 2025

Conversation

yellowhatter
Copy link
Contributor

@yellowhatter yellowhatter commented Feb 4, 2025

Adds SHM lazy init behavior control through config

"lazy" was the only behavior of SHM before this PR: initialize SHM internal data and threads on-demand (which means "at first SHM buffer interaction"). This PR adds "init" mode that initializes all SHM stuff at first Session open.

From the config doc:

/// - "lazy": SHM subsystem internals will be initialized lazily upon the first SHM buffer
/// allocation or reception. This setting provides better startup time and optimizes resource usage,
/// but produces extra latency at the first SHM buffer interaction.
/// - "init": SHM subsystem internals will be initialized upon Session opening. This setting sacrifices
/// startup time, but guarantees no latency impact when first SHM buffer is processed.

@yellowhatter yellowhatter added new feature Something new is needed release Part of the next release labels Feb 4, 2025
@yellowhatter yellowhatter requested a review from Mallets February 4, 2025 14:09
@yellowhatter yellowhatter self-assigned this Feb 4, 2025
/// latency at the first SHM buffer interaction moment.
/// `false` setting sacrifices startup time, but guarantees no latency impact when first SHM buffer is
/// processed.
lazy_init: bool,
Copy link
Member

Choose a reason for hiding this comment

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

In the queue, the lazy initialisation is exposed with this configuration structure:

allocation: {

If possible it would be good to adopt a similar structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@Mallets
Copy link
Member

Mallets commented Feb 5, 2025

It's unclear to me from this PR who the lazy init workflow behaves. Would you mind elaborating it a bit?

@yellowhatter
Copy link
Contributor Author

It's unclear to me from this PR who the lazy init workflow behaves. Would you mind elaborating it a bit?

"lazy" was the only behavior of SHM before this PR: initialize SHM internal data and threads on-demand (which means "at first SHM buffer interaction"). This PR adds "init" mode that initializes all SHM stuff at first Session open.

From the config doc:

/// - "lazy": SHM subsystem internals will be initialized lazily upon the first SHM buffer
/// allocation or reception. This setting provides better startup time and optimizes resource usage,
/// but produces extra latency at the first SHM buffer interaction.
/// - "init": SHM subsystem internals will be initialized upon Session opening. This setting sacrifices
/// startup time, but guarantees no latency impact when first SHM buffer is processed.

@Mallets
Copy link
Member

Mallets commented Feb 5, 2025

It's unclear to me from this PR who the lazy init workflow behaves. Would you mind elaborating it a bit?

"lazy" was the only behavior of SHM before this PR: initialize SHM internal data and threads on-demand (which means "at first SHM buffer interaction"). This PR adds "init" mode that initializes all SHM stuff at first Session open.

From the config doc:

/// - "lazy": SHM subsystem internals will be initialized lazily upon the first SHM buffer
/// allocation or reception. This setting provides better startup time and optimizes resource usage,
/// but produces extra latency at the first SHM buffer interaction.
/// - "init": SHM subsystem internals will be initialized upon Session opening. This setting sacrifices
/// startup time, but guarantees no latency impact when first SHM buffer is processed.

Thanks! I thought more things were needed to add the desired behaviour but it's not the case.

LGTM I would then simply address the comment on the configuration.

@Mallets Mallets merged commit ca058bd into eclipse-zenoh:main Feb 5, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Something new is needed release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants