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
Cleanup up and Fixing component creation and deletion #2998
Conversation
def8951
to
6157a01
Compare
- removing operator new/delete overloads - removing functions to retreive component types
6157a01
to
0db4a50
Compare
// | ||
// return type; | ||
// } | ||
// } |
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.
Can this be removed?
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.
Yes.
inline component_type get_component_type() | ||
{ | ||
// return detail::get_component_type_impl< | ||
// typename hpx::util::decay<Component>::type>(nullptr); |
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.
Can this be removed?
HPX_THROW_EXCEPTION(hpx::unknown_component_address, | ||
"create<Component>", | ||
"can't assign global id"); | ||
void *storage = component_heap<Component>().alloc(1); |
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.
Does this throw if it fails to allocate memory?
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.
It might throw. In the case of an exception, it is propagated up to the caller. This is the same behavior as when the Component
ctor throws.
throw; | ||
return naming::invalid_gid; |
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.
Why do you need the return?
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.
Good catch.
throw; | ||
return naming::invalid_gid; |
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.
Why do you need the return?
{ | ||
HPX_ASSERT(parcels_.size() == handlers_.size()); | ||
for(std::size_t i = 0; i < parcels_.size(); ++i) | ||
{ | ||
handlers_[i](e, parcels_[i]); | ||
handlers_[i].reset(); | ||
} |
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.
Is this change related to this PR?
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.
Not directly... Not sure why I included it. Potentially, it leads to resources being freed in a timelier fashion. I could add it in a different PR if you wish.
} | ||
type = derived_component_type(type, base_type); | ||
// components::enabled(components::get_derived_type(type)) = enabled; | ||
// components::enabled(components::get_base_type(type)) = enabled; |
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.
Can this be removed?
using components::stubs::runtime_support; | ||
agas::gva g (addr.locality_, addr.type_, 1, addr.address_); | ||
runtime_support::free_component_sync(g, gid, 1); | ||
naming::address addr(addr_); |
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.
Why do you need a local copy?
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.
The local copy is needed because agas::is_local_address_cached
requires an lvalue.
This patch contains various fixes related to component registration, creation and deletion. The main motivation was to allow direct creation of components without going through a factory function, disallowing perfect forwarding for local component creation. The component registration has been cleaned up, the factory class for creation and deletion of components has been removed, and the component type registration has been moved to the component_registry. The component deletion has been moved out of the runtime_support server, since no factory is required anymore. Various other cleanups have been performed leading to an overall reduction in binary size and removal of various different duplicate code while preserving existing functionality. Fixes include: - The functionality of component instance counters has been fixed for local_new - The deletion of components now correctly takes migrated components into account - The component heap instances are now ensured to live in single module only avoiding freeing memory that has been allocated by a different module.
0db4a50
to
a7fc77a
Compare
The comments have been addressed. |
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.
LGTM, thanks!
This patch contains various fixes related to component registration, creation
and deletion.
The main motivation was to allow direct creation of components without going
through a factory function, disallowing perfect forwarding for local component
creation.
The component registration has been cleaned up, the factory class for creation
and deletion of components has been removed, and the component type registration
has been moved to the component_registry.
The component deletion has been moved out of the runtime_support server, since
no factory is required anymore.
Various other cleanups have been performed leading to an overall reduction in
binary size and removal of various different duplicate code while preserving
existing functionality.
Fixes include:
local_new
into account
avoiding freeing memory that has been allocated by a different module.
Other flyby changes include some cleanup of the runtime_support server and the component interfaces.