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

Add a function to pre-set infos for all streams/devices/... #665

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

therault
Copy link
Contributor

@therault therault commented Aug 7, 2024

Lazy/dynamic allocation of infos can be convenient for the users, but also detrimental to predictive behaviors. Examples of drawbacks are when testing memory constrained setups (e.g., when the test uses zone_malloc to dynamically allocate objects in the info object constructor, that may fail nondeterministically because it depends how many streams call the constructor), or small scale performance runs where the number of cublas objects to initialize vary depending on the run. Calling the new function incurs deterministic overheads.

Lazy/dynamic allocation of infos can be convenient for the users,
but also detrimental to predictive behaviors. Examples of drawbacks
are when testing memory constrained setups (e.g., when the test uses
zone_malloc to dynamically allocate objects in the info object
constructor, that may fail undeterministically because it depends
how many streams call the constructor), or small scale performance
runs where the number of cublas objects to initialize vary depending
on the run. Calling the new function incurs determinisitic overheads.
@therault therault requested a review from a team as a code owner August 7, 2024 14:51
…ser's object info constructor (and potentially destructor). Allow the device to define a function to do so in order to keep the user code unchanged. This adds an additional function pointer call, but this call only happens once per info and per stream, ideally during warmup/initialization time
nio = ie->constructor(oa->cons_obj, ie->cons_data);
ret = parsec_info_test_and_set(oa, iid, nio, NULL);
if(ret != nio && NULL != ie->destructor) {
ie->destructor(nio, ie->des_data);
}
return ret;
}

void parsec_info_set_all(parsec_info_t *nfo, parsec_info_id_t iid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest a different name:

Suggested change
void parsec_info_set_all(parsec_info_t *nfo, parsec_info_id_t iid)
void parsec_info_populate(parsec_info_t *nfo, parsec_info_id_t iid)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me question the need for this function. It is only use in a single location, and based on what it is doing I don't see the need for a capability to populate the entire info set. Second issue with this populate capability, is that all memory allocation will be done in the context of the caller of the populate, instead of allowing the allocated objects to belong to the scope of the thread/stream that would use them.

In fact my main issue is with the entire info API. Because parsec_info_set nicely allows you to set a value for a key, but then parsec_info_get will magically allocate one for you even if you never set one. And there is no way to distinguish between the case where the value was properly set and the case where it was auto-generated.

@abouteiller
Copy link
Contributor

Oct. 25. We decided this is a useful feature but it needs a more descriptive name and maybe have functionality to pre-populate everything that needs pre-allocated

@abouteiller abouteiller marked this pull request as draft October 25, 2024 15:07
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.

4 participants