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

fix #514, call_data_callbacks after shared the data #548

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

fix #514, call_data_callbacks after shared the data #548

wants to merge 17 commits into from

Conversation

jmorice91
Copy link
Contributor

@jmorice91 jmorice91 commented Mar 2, 2025 •
edited
Loading

!!!INSERT YOUR DESCRIPTION HERE!!!

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@jmorice91 jmorice91 linked an issue Mar 2, 2025 that may be closed by this pull request
Delay data events in multi_expose to when the last data is exposed #514
Open
@jmorice91 jmorice91 marked this pull request as draft March 2, 2025 21:48
@jmorice91 jmorice91 marked this pull request as ready for review March 13, 2025 21:43
@jmorice91 jmorice91 self-assigned this Mar 13, 2025
benoitmartin88
benoitmartin88 requested changes Mar 20, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

Great feature to add. Thanks !
Not much is needed to merge this PR.
I think this feature would require a unit test. Could you add this?

pdi/src/pdi.cxx Outdated
if ((status = PDI_share(v_name, v_data, v_access))) {
break;

Var_to_reclaim list_reclaim;
Copy link
Member

Choose a reason for hiding this comment

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

Add comment to explain what this does.

pdi/src/data_descriptor_impl.h
void* share(Ref ref, bool read, bool write) override;
void* share(Ref ref, bool read, bool write, bool delayDataCallback = false) override;

void data_callbacks() override;
Copy link
Member

Choose a reason for hiding this comment

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

Is this new method really needed ?
It is only used once. If the goal is to factorize calls to call_data_callbacks, than it should be used everywhere where call_data_callbacks is used.

pdi/src/pdi.cxx Outdated
break;

Var_to_reclaim list_reclaim;
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{

I don't think this is needed

pdi/src/pdi.cxx
for (auto&& it = list_reclaim.begin(); it != list_reclaim.end(); it++) {
Global_context::context().logger().trace("Multi expose: data events `{}' ({}/{})", it->c_str(), ++i, list_reclaim.size());
Global_context::context()[it->c_str()].data_callbacks();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}

pdi/src/pdi.cxx
for (auto&& it = list_reclaim.begin(); it != list_reclaim.end(); it++) {
Global_context::context().logger().trace("Multi expose: data events `{}' ({}/{})", it->c_str(), ++i, list_reclaim.size());
Global_context::context()[it->c_str()].data_callbacks();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}

@jmorice91 jmorice91 marked this pull request as draft April 4, 2025 08:43
@jmorice91 jmorice91 force-pushed the 514-delay-data-events-in-multi_expose branch from 656e28b to 3e1d45f Compare April 4, 2025 09:23
@jmorice91 jmorice91 requested a review from JAuriac April 9, 2025 13:27
@jmorice91 jmorice91 marked this pull request as ready for review April 9, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbigot jbigot Awaiting requested review from jbigot

@Yushan-Wang Yushan-Wang Awaiting requested review from Yushan-Wang

@JAuriac JAuriac Awaiting requested review from JAuriac

@benoitmartin88 benoitmartin88 Awaiting requested review from benoitmartin88

Requested changes must be addressed to merge this pull request.

Assignees

@jmorice91 jmorice91

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Delay data events in multi_expose to when the last data is exposed
2 participants
@jmorice91 @benoitmartin88