-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
Conversation
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.
…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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest a different name:
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) |
There was a problem hiding this comment.
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.
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 |
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.