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

Optimizing LCO continuations #1980

Merged
merged 10 commits into from Feb 13, 2016

Conversation

Projects
None yet
2 participants
@sithhell
Copy link
Member

commented Feb 10, 2016

Instead of routing set_value/set_error/trigger_event actions to the
destination, we send the lva address along with the continuation to
avoid further AGAS lookups

@sithhell sithhell added this to the 0.9.12 milestone Feb 10, 2016

@sithhell sithhell force-pushed the optimizing_continuations branch from 5440e19 to da9771d Feb 10, 2016

Optimizing LCO continuations
Instead of routing set_value/set_error/trigger_event actions to the
destination, we send the lva address along with the continuation to
avoid further AGAS lookups
return naming::address(
hpx::get_locality()
, impl_->get_component_type()
, &*impl_

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

I'd prefer writing this as impl_.get().

return naming::address(
hpx::get_locality()
, impl_->get_component_type()
, &*impl_

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Same as above.

{
if(addr.locality_ == hpx::get_locality())
{
std::pair<bool, components::pinned_ptr> r;

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Shouldn't this definition be moved out of the if() {} block? Also, wouldn't the trait be called twice if it returns true the first time?

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 10, 2016

Author Member

You are right, the definition has to be removed out of the if block.
The trait has to go inside the if block though, i guess.

if(addr.locality_ == hpx::get_locality())
{
std::pair<bool, components::pinned_ptr> r;
r = traits::action_was_object_migrated<Action>::call(

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Same as above.

@@ -10,6 +10,7 @@

#include <hpx/runtime/actions/continuation.hpp>

/*

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Was this removed intentionally? Or is it an oversight?

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 10, 2016

Author Member

Yes, this was removed intentionally. the whole file could go as those functions aren't used anywhere.

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Those are API functions, I'd say we should leave them in. They are useful in their own right.

@@ -70,7 +70,7 @@ int hpx_main(int argc, char* argv[])

// This segfaults, because the promise is not alive any more.
// It SHOULD get kept alive by AGAS though.
hpx::set_lco_value(promise_id, 10, false);
hpx::set_lco_value(promise_id, naming::address(), 10, false);

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

Would it be possible to leave the old API in place?

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 10, 2016

Author Member

Ok, as you wish.

@@ -47,7 +47,7 @@ managed_refcnt_checker::~managed_refcnt_checker()
strm << ( boost::format("[%1%/%2%]: triggering flag %3%\n")
% prefix_ % this_ % target_);

hpx::trigger_lco_event(target_);
hpx::trigger_lco_event(target_, naming::address());
}

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 10, 2016

Member

I'd like to leave the old API in place here as well. Having to pass a default constructed address() is confusing.

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2016

The review comments have been addressed.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

[10 Feb 16 06:40] * hkaiser * heller: couldn't the continuations take the address by &&?
[10 Feb 16 06:41] * heller * hkaiser: most likely they could indeed
[10 Feb 16 06:43] * heller * such that we move the address out of the continuation?
[10 Feb 16 06:43] * hkaiser * well, we know it's used only once, don't we?
[10 Feb 16 06:43] * heller * i think so, yes

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

Do you want to see that implemented right away or should we rather merge it now (after circle ci passes) and create a ticket about fixing this so we implement it later on?

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

The reason why I ask is because we could additionally do the same for the GID.

@hkaiser

This comment has been minimized.

Copy link
Member

commented on 6c6092b Feb 11, 2016

Isn't this dangerous? Different compilers might use different packing schemes for the consecutive data members...

This comment has been minimized.

Copy link
Member Author

replied Feb 11, 2016

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

Isn't this dangerous? Different compilers might use different packing schemes for the consecutive data members...

Good point, don't we have some guarantee for that? Also, what's the difference to name_serialization_data defined in address.cpp or gid_type?

You're right, we probably never have attempted to pay attention to this issue. The same problem applies to hpx::util::tuple<Ts...>, (for Ts being bitwise serializable)...

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

Do you want to see that implemented right away or should we rather merge it now (after circle ci passes) and create a ticket about fixing this so we implement it later on?

We move naming::address everywhere, so I'd say let's fix this. Passing id_type by rvalue does not buy us too much, so this can wait.

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2016

We move naming::address everywhere, so I'd say let's fix this. Passing id_type by rvalue does not buy us too much, so this can wait.

done.

@@ -12,7 +12,7 @@
///////////////////////////////////////////////////////////////////////////////
void test(hpx::id_type const& id)
{
hpx::trigger_lco_event(id);
hpx::trigger_lco_event(id, hpx::naming::address());

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 12, 2016

Member

Minor nitpick: this change is not necessary anymore.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 12, 2016

LGTM! Please go ahead and merge.

Passing naming::address by &&
Flyby: reverting change to set_parcel_write_handler.cpp

@sithhell sithhell force-pushed the optimizing_continuations branch from c68e72f to f421331 Feb 12, 2016

hkaiser added a commit that referenced this pull request Feb 13, 2016

@hkaiser hkaiser merged commit 5e4a0be into master Feb 13, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@hkaiser hkaiser deleted the optimizing_continuations branch Feb 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.